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

[Blaž Tomažič]

Hi,

On pon, 2012-12-03 at 09:41 -0600, Aaron Watry wrote:
> Hi Tom/Blaz/Piglit,
> 
> Recently, when I've been working on changes to the piglit CL tests,
> I've usually had to run with a modified version of
> piglit-framework-cl-program.c
> 
> 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.

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?

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

> 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