[Piglit] [RFC PATCH] Only specify -cl-std when test hasn't explicitly overridden -cl-std
Blaž Tomažič
blaz.tomazic at gmail.com
Fri Dec 7 06:22:09 PST 2012
On čet, 2012-12-06 at 14:29 -0600, Aaron Watry wrote:
> 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.
It's true. But in this case what we are checking is that we are
*compiling* on 1.0 device, not that we are *testing* 1.0 compliance.
For example, if we want to test just OpenCL 1.0 conformance on a 1.1
device we can't do it this way. There is no way I know of to tell the
compiler to compile the test for 1.0 version (CL1.0 parameter for
-cl-std is not defined).
It goes in the other way too, we also can't test OpenCL 1.1 conformance
on 1.0 device this way (I guess this will often be used in driver
development, so you don't need to change the device version string just
to test if your newly implemented 1.1 feature works).
I wrongly said before that we should replace CL_VERSION_X_X with
PIGLIT_CL_VERSION_X_X. PIGLIT_CL_VERSION_X_X would only be used for
__PIGLIT_CL_VERSION__ and __PIGLIT_CLC_VERSION__, so we don't have to
check for existence of CL_VERSION_X_X. CL_VERSION_X_X should still be
used for __OPENCL_VERSION__ and __OPENCL_C_VERSION__.
> 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.
With the new macros, testing the preprocessor would be easy. Example:
#if __PIGLIT_CLC_VERSION__ == PIGLIT_CL_VERSION_1_2
#if defined(CL_VERSION_1_0) && CL_VERSION_1_0 == 100 && \
defined(CL_VERSION_1_1) && CL_VERSION_1_1 == 110 && \
defined(CL_VERSION_1_2) && CL_VERSION_1_2 == 120 && \
defined(__OPENCL_C_VERSION__) && \
__OPENCL_C_VERSION__ == FROM_clGetDeviceInfo_OR_cl_std
*out = 1;
#else
*out = 0;
#endif
#endif
> > 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_*.
Then I will create a patch for __PIGLIT_CL[C]_VERSION__ macros to see
how it works.
I think that we should add PIGLIT_CL_VERSION_1_* so we can compare
__PIGLIT_CL[C]_VERSION__ to something instead of raw numbers or adding
checking for existence CL_VERSION_1_X. I will try do that in a separate
patch, so we can throw it away if we're not OK with it.
Blaž
> --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