[Piglit] [RFC PATCH] Only specify -cl-std when test hasn't explicitly overridden -cl-std
Aaron Watry
awatry at gmail.com
Thu Dec 6 12:29:32 PST 2012
On Thu, Dec 6, 2012 at 7:16 AM, Blaž Tomažič <blaz.tomazic at gmail.com> wrote:
> Sorry for late response and a long post, but I had to give much more
> thought to this problem.
>
> I'll try to start from the beginning. That piece of code that you
> patched and the other that you commented out was supposed to ease the
> development of kernel tests so you could have one test for multiple
> versions of OpenCL C:
> #if OPENCL_C_1_0
> ...
> #elif OPENCL_C_1_1
> ...
> #else
> ...
> #endif
>
> As you found out, the current implementation of this functionality is
> bad :). It redefines macros from higher versions of OpenCL
> (__OPENCL_C_VERSION__ from 1.2) to be available to lower versions, which
> can lead to a compilation fail if a macro is already defined. And it
> uses -cl-std compiler option to set the value of __OPENCL_C_VERSION__.
> Also, as I recently found out, CL1.0 is not a valid value for -cl-std
> (only CL1.1 and CL1.2), so the current solution can't even test just 1.0
> compliance on higher versions.
>
> I think that we should remove this piece of code since we really should
> not pollute the environment that is there for OpenCL implementations to
> create, and reconsider the problem.
>
Sounds good to me. We'll try to come up with a better solution than
what we have currently.
> All we want to do with kernel tests is to test specific OpenCL C version
> compliance. So we should simply signal the kernel tests which version we
> want to test. We can do this by passing our own macros:
> * __PIGLIT_CL_VERSION__
> * __PIGLIT_CLC_VERSION__
> * PIGLIT_CL_VERSION_1_0, PIGLIT_CL_VERSION_1_1,...
> By using these macros each peace of code will exactly know what it needs
> to test and also which OpenCL C macros in the spec should be available
> to it.
> Also each kernel test would be compiled with the latest version of
> available OpenCL C compiler, we would not use -cl-std. If that is
> needed, tests can always pass compiler options to the framework.
>
> Explanations of each macro:
> * __PIGLIT_CL_VERSION__: Tells which version of OpenCL we are
> testing. It does not replace __OPENCL_VERSION__, since this
> tells us which OpenCL version the device supports.
> * __PIGLIT_CLC_VERSION__: Tells which version of OpenCL C we are
> testing. This is also not a replacement for
> __OPENCL_C_VERSION__, since this tells us with which OpenCL C
> version of the compiler are the kernels compiled. This way we
> can test 1.0 or 1.1 compliance on a compiler that reports
> version 1.2.
> * PIGLIT_CL_VERSION_1_0,...: This macros do replace
> CL_VERSION_1_0,... macros. This is needed because OpenCL 1.0
> does not specify declaration of CL_VERSION_1_0, which leads to a
> problem where "#if __PIGLIT_CL_VERSION__ == CL_VERSION_1_0"
> evaluates to false on OpenCL C 1.0.
>
Regarding __PIGLIT_CL_VERSION__ and CL_VERSION_1_0... In previous CL
code that I've written which needed to run on CL 1.0, I've gotten
around this by putting the following in my CL source:
#if !defined(__CL_VERSION_1_1__) || (__OPENCL_VERSION__ < __CL_VERSION_1_1__)
//perform conditional declarations of macros/functions so that the
rest of the code can run as if __OPENCL_VERSION__ was 1.0
#endif
We could do something similar here. OpenCL 1.0 doesn't define
CL_VERSION_1_0, which means that if CL_VERSION_1_0/CL_VERSION_1_1
aren't defined we should be able to assume we're running on 1.0.
Of course, if we're attempting to test if the run-time is properly
defining these macros, we'd have to have additional tests that
verified that.
> By using these macros we can also safely use macros which are defined by
> the OpenCL implementation:
> * __OPENCL_C_VERSION__ (check __PIGLIT_CLC_VERSION__ >=
> PIGLIT_CL_VERSION_1_2)
> * CL_VERSION_1_0 (check __PIGLIT_CLC_VERSION__ >=
> PIGLIT_CL_VERSION_1_1)
>
> Some tests will become more verbose this way, but I think that this is a
> more clean solution than trying to define __OPENCL_C_VERSION__ for lower
> versions (<1.2) of OpenCL implementations, as we do now. Even if we
> introduce __OPENCL_C_VERSION__ macro without creating compilation
> problems, we are then creating tests that use a macro of unknown origin.
> We should avoid doing that, because there should be a clear distinction
> between Piglit defined macros and OpenCL defined ones, so I think the
> solutions with new macros is a better one.
>
> Thoughts?
I agree that we shouldn't be defining CL-standard macros on behalf of
the implementations, they should be doing that themselves. I'm not yet
sure what the proper solution is, but I can see the logic behind
adding the new PIGLIT_CL_* macros, with the exception of
PIGLIT_CL_VERSION_1_*.
--Aaron
>
> On tor, 2012-12-04 at 12:55 -0600, Aaron Watry wrote:
>> On Tue, Dec 4, 2012 at 8:43 AM, Blaž Tomažič <blaz.tomazic at gmail.com> wrote:
>>
>> >> In general, I've had to comment out all of the following code in order
>> >> to get the Nvidia and Apple implementations to run things for many
>> >> tests.
>> >>
>> >> Around line 225:
>> >> if(env.clc_version <= 10) {
>> >> char* template = "-D CL_VERSION_1_0=100 ";
>> >> char* old = build_options;
>> >> build_options = malloc((strlen(old) + strlen(template) + 1) *
>> >> sizeof(char));
>> >> strcpy(build_options, old);
>> >> sprintf(build_options+strlen(old), template);
>> >> free(old);
>> >> }
>> >> if(env.clc_version <= 11) {
>> >> char* template = "-D __OPENCL_C_VERSION__=1%d0 ";
>> >> char* old = build_options;
>> >> build_options = malloc((strlen(old) + strlen(template) + 1) *
>> >> sizeof(char));
>> >> strcpy(build_options, old);
>> >> sprintf(build_options+strlen(old), template, env.clc_version%
>> >> 10);
>> >> free(old);
>> >> }
>> >>
>> >>
>> >> I'm of the opinion that we should remove these two IF blocks
>> >> altogether. When the CL version is 1.0, these macros are not required
>> >> to be defined (at least they weren't in CL 1.0). Also, by defining
>> >> CL_VERSION_1_0 here, we actually redefine the existing macro on the
>> >> Apple implementation, which makes ALL tests fail to build with an
>> >> error. Part of me says that we should only be overriding the version
>> >> declarations through "-cl-std=" in the test's build options.
>> >
>> > The purpose of this part of the code was to add two macros to OpenCL
>> > implementations that don't have them:
>> > * OpenCL 1.0 [0] is missing CL_VERSION_1_0 and
>> > __OPENCL_C_VERSION__
>> > * OpenCL 1.1 [1] is missing __OPENCL_C_VERSION__
>> > so you could safely use them in OpenCL C programs (usage example is
>> > presented below). I guess I didn't consider what would happen if those
>> > macros were already defined :).
>> >
>> > I agree that we remove the (re)definition of CL_VERSION_1_0, because it
>> > doesn't serve any purpose. If there is no CL_VERSION_1_1 defined we know
>> > we are on 1.0.
>>
>> Agreed. Most of my previous work checked for CL 1.0 this way. I'll
>> submit a patch for this at some point (hopefully later today).
>>
>> >
>> > But not defining __OPENCL_C_VERSION__ creates a problem on OpenCL 1.1
>> > implementations. In this case the OpenCL C program doesn't have any
>> > macro to alter it's code flow depending with which version it was
>> > compiled (1.0 or 1.1). A great example of using this functionality is
>> > program/build/vector-data-types.cl. In current form type3 vectors would
>> > not be tested on OpenCL 1.1 platforms. (You could check for
>> > CL_VERSION_1_1 (platform version) but what if the program was compiled
>> > with version 1.0 because the device doesn't support 1.1?)
>> >
>> > I see two options how to solve this problem:
>> > 1. Define __OPENCL_C_VERSION__ *only* on OpenCL 1.1/1.0 platforms.
>> > We are currently defining __OPENCL_C_VERSION__ if we are testing
>> > OpenCL C 1.1/1.0 conformance. We should instead define it if the
>> > platform reports that its version is 1.1 or 1.0. Problem: OpenCL
>> > 1.1/1.0 implementation could still have __OPENCL_C_VERSION__
>> > version already defined. Should we try to detect if it defines
>> > __OPENCL_C_VERSION__ with a small OpenCL C program, before
>> > compiling the test?
>> > 2. Add a new define: __PIGLIT_CLC_VERSION__. It would serve the
>> > same purpose as __OPENCL_C_VERSION__, but with a main difference
>> > that it would always be defined. (In this case we can also add
>> > __PIGLIT_CL_VERSION__ which tells against which OpenCL version
>> > we are testing. I don't see any usage for it, but I guess it
>> > doesn't hurt adding it for the sake of completeness). Problem:
>> > OpenCL C programs have now dependencies on non-'OpenCL spec'
>> > constructs.
>> >
>> > Any thoughts on this?
>>
>> Of the two options, I like the 1st better. For now, I'll just remove
>> the first IF block (defining CL_VERSION_1_0) and re-test as is. If I'm
>> still experiencing problems, I'll see how hard it is to implement a
>> small program which checks for the definition of __OPENCL_C_VERSION__
>> when CL version is <= 1.1.
>
> I also liked the 1st option more at the beginning, but now I think the
> second one is better.
>
>> Regarding the definition of __OPENCL_C_VERSION__, do you know what the
>> prevailing behavior is for the definition of __OPENCL_VERSION__ within
>> kernels when the CL version is 1.1, and the device only supports 1.0?
>> Will __OPENCL_VERSION__ be defined as 11 or 10? When I was previously
>> attempting to use char3/int3 in CL 1.1, I usually just checked the
>> __OPENCL_VERSION__ and if it reported CL_VERSION_1_1 things worked
>> out... even if that was possibly incorrect.
>
> The __OPENCL_VERSION__ macro should in that case be 10, if I understand
> the spec correctly. __OPENCL_VERSION__ tells you the OpenCL version of a
> device and __OPENCL_C_VERSION__ the OpenCL C used to compile the
> program. You can alter OpenCL C version used by -cl-std, so checking
> __OPENCL_VERSION__ for OpenCL C 1.1 functionality is wrong, you should
> use __OPENCL_C_VERSION__. Or if the proposed solution above is accepted,
> check for __PIGLIT_CLC_VERSION__.
>
>> >
>> >> At that point, we can add tests to make sure that the implementation
>> >> respects the requested version, and we can stop assuming that
>> >> redefining those macros actually does what we want.
>> >>
>> >> Additionally, I've attached a patch to the
>> >> piglit-framework-cl-program.c which only inserts " -cl-std=CL%d.%d"
>> >> when the test's build_options configuration hasn't explicitly
>> >> overridden the -cl-std value already. Without this, it is possible to
>> >> have a build_options line that looks like "-cl-std=CL1.1
>> >> -cl-std=CL1.2", which is very likely wrong.
>> >
>> > The patch looks fine to me. We should really avoid passing -cl-std
>> > twice.
>> >
>> > But it should be noted that it's always better to use clc_version_min
>> > and clc_version_max test arguments instead of directly defining -cl-std.
>> > This way the test is *skipped* if the device doesn't support high enough
>> > version of OpenCL C, while using only -cl-std would in this case mark
>> > test as *failed* because it probably would not compile.
>> > And if the device version is high enough (higher or equal to
>> > clc_version_min) the framework adds -cl-std=CLx.x that is not higher
>> > than clc_version_max or the max version that the device supports.
>>
>>
>> I agree that we should always be using clc_version_[min|max] to define
>> when the tests run (and I've been doing this in the tests I've been
>> writing). In this case, the specific test that I was trying to run is
>> testing the validity of the argument passed in '-cl-std=', which
>> depends on piglit not messing with it.
>>
>> The test is program/build/fail/invalid-version-declaration.cl, and I'm
>> explicitly passing '-cl-std=x.1' in the hopes of getting a compiler
>> failure.
>
> I see, the new proposed solution would not mess with -cl-std, so this
> would work as wanted.
>
>> Thanks again for the feedback,
>> Aaron
>>
>>
>> >
>> >> Anyway, let me know what you think. I'd like to come to some sort of
>> >> conclusion on whether Apple/Nvidia are wrong, or whether piglit needs
>> >> some tweaks.
>> >>
>> >> --Aaron
>> >
>> > Blaž
>> >
>> > [0] https://www.khronos.org/registry/cl/sdk/1.0/docs/man/xhtml/preprocessorDirectives.html
>> > [1] https://www.khronos.org/registry/cl/sdk/1.1/docs/man/xhtml/preprocessorDirectives.html
>> >
>> >
>
> Blaž
>
More information about the Piglit
mailing list