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

Blaž Tomažič blaz.tomazic at gmail.com
Thu Dec 6 05:16:47 PST 2012

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

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.

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

By using these macros we can also safely use macros which are defined by
the OpenCL implementation:
      * __OPENCL_C_VERSION__ (check __PIGLIT_CLC_VERSION__ >=
      * CL_VERSION_1_0 (check __PIGLIT_CLC_VERSION__ >=

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.


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


More information about the Piglit mailing list