[Piglit] [RFC PATCH] Only specify -cl-std when test hasn't explicitly overridden -cl-std

[Aaron Watry]

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.

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.

>
>> 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.

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
>
>