[Mesa-dev,8/8] clover/llvm: Make __OPENCL_VERSION__ dynamic

Submitted by Aaron Watry on July 31, 2017, 1:26 a.m.

Details

Message ID 20170731012612.23176-9-awatry@gmail.com
State New
Headers show
Series "A few clover fixes for both CTS and eventual 1.2 support" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Aaron Watry July 31, 2017, 1:26 a.m.
Signed-off-by: Aaron Watry <awatry@gmail.com>
CC: Jan Vesely <jan.vesely@rutgers.edu>

v2: base it on the device version
---
 src/gallium/state_trackers/clover/llvm/invocation.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
index 63b2961752..443cd31e66 100644
--- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
+++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
@@ -224,7 +224,8 @@  namespace {
       c.getPreprocessorOpts().Includes.push_back("clc/clc.h");
 
       // Add definition for the OpenCL version
-      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=110");
+      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=" +
+              std::to_string(get_language_from_version_str(dev.device_version())));
 
       // clc.h requires that this macro be defined:
       c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");

Comments

On Sun, 2017-07-30 at 20:26 -0500, Aaron Watry wrote:
> Signed-off-by: Aaron Watry <awatry@gmail.com>
> CC: Jan Vesely <jan.vesely@rutgers.edu>
> 
> v2: base it on the device version
> ---
>  src/gallium/state_trackers/clover/llvm/invocation.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index 63b2961752..443cd31e66 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -224,7 +224,8 @@ namespace {
>        c.getPreprocessorOpts().Includes.push_back("clc/clc.h");
>  
>        // Add definition for the OpenCL version
> -      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=110");
> +      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=" +
> +              std::to_string(get_language_from_version_str(dev.device_version())));

I don't think you can use the same parsing function here.
__OPENCL_VERSION__ can go up to 2.2, while __OPENCL_C_VERSION__ is max
2.0

Jan

>  
>        // clc.h requires that this macro be defined:
>        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
On Fri, Aug 4, 2017 at 1:43 PM, Jan Vesely <jan.vesely@rutgers.edu> wrote:
> On Sun, 2017-07-30 at 20:26 -0500, Aaron Watry wrote:
>> Signed-off-by: Aaron Watry <awatry@gmail.com>
>> CC: Jan Vesely <jan.vesely@rutgers.edu>
>>
>> v2: base it on the device version
>> ---
>>  src/gallium/state_trackers/clover/llvm/invocation.cpp | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> index 63b2961752..443cd31e66 100644
>> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> @@ -224,7 +224,8 @@ namespace {
>>        c.getPreprocessorOpts().Includes.push_back("clc/clc.h");
>>
>>        // Add definition for the OpenCL version
>> -      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=110");
>> +      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=" +
>> +              std::to_string(get_language_from_version_str(dev.device_version())));
>
> I don't think you can use the same parsing function here.
> __OPENCL_VERSION__ can go up to 2.2, while __OPENCL_C_VERSION__ is max
> 2.0

This is what I get for still only reading the 1.2 spec when I'm
working on these things.  I've been working on getting us up to
better/reasonable CL 1.2 CTS pass rates, and haven't been looking
ahead much at all.

>
> Jan
>
>>
>>        // clc.h requires that this macro be defined:
>>        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
On Fri, Aug 4, 2017 at 1:43 PM, Jan Vesely <jan.vesely@rutgers.edu> wrote:
> On Sun, 2017-07-30 at 20:26 -0500, Aaron Watry wrote:
>> Signed-off-by: Aaron Watry <awatry@gmail.com>
>> CC: Jan Vesely <jan.vesely@rutgers.edu>
>>
>> v2: base it on the device version
>> ---
>>  src/gallium/state_trackers/clover/llvm/invocation.cpp | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> index 63b2961752..443cd31e66 100644
>> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> @@ -224,7 +224,8 @@ namespace {
>>        c.getPreprocessorOpts().Includes.push_back("clc/clc.h");
>>
>>        // Add definition for the OpenCL version
>> -      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=110");
>> +      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=" +
>> +              std::to_string(get_language_from_version_str(dev.device_version())));
>
> I don't think you can use the same parsing function here.
> __OPENCL_VERSION__ can go up to 2.2, while __OPENCL_C_VERSION__ is max
> 2.0

Is that an issue here? I thought that the device's highest supported
OpenCL version was what was required here?

__OPENCL_VERSION__ is defined as "substitutes an integer reflecting
the version number of the OpenCL
supported by the OpenCL device", which in this case should map
directly to dev.device_version().

__OPENCL_C_VERSION__ is already added by clang when the pre-processor
is initialized using the selected language version
(clang/FrontEnd/InitPreprocessor.cpp).

--Aaron

>
> Jan
>
>>
>>        // clc.h requires that this macro be defined:
>>        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
On Wed, 2017-08-09 at 22:36 -0500, Aaron Watry wrote:
> On Fri, Aug 4, 2017 at 1:43 PM, Jan Vesely <jan.vesely@rutgers.edu> wrote:
> > On Sun, 2017-07-30 at 20:26 -0500, Aaron Watry wrote:
> > > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > > CC: Jan Vesely <jan.vesely@rutgers.edu>
> > > 
> > > v2: base it on the device version
> > > ---
> > >  src/gallium/state_trackers/clover/llvm/invocation.cpp | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > index 63b2961752..443cd31e66 100644
> > > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > @@ -224,7 +224,8 @@ namespace {
> > >        c.getPreprocessorOpts().Includes.push_back("clc/clc.h");
> > > 
> > >        // Add definition for the OpenCL version
> > > -      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=110");
> > > +      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=" +
> > > +              std::to_string(get_language_from_version_str(dev.device_version())));
> > 
> > I don't think you can use the same parsing function here.
> > __OPENCL_VERSION__ can go up to 2.2, while __OPENCL_C_VERSION__ is max
> > 2.0
> 
> Is that an issue here? I thought that the device's highest supported
> OpenCL version was what was required here?

The problem is that 'get_language_from_version_str' throws exception
when the version is >2.0. It's OK for clc version, but device CL
version can go above that.
I don't think it's a big problem, a simple "TODO: consider higher
versions" might be enough for now. it will take a while for clover to
actually run into this.

> 
> __OPENCL_VERSION__ is defined as "substitutes an integer reflecting
> the version number of the OpenCL
> supported by the OpenCL device", which in this case should map
> directly to dev.device_version().
> 
> __OPENCL_C_VERSION__ is already added by clang when the pre-processor
> is initialized using the selected language version
> (clang/FrontEnd/InitPreprocessor.cpp).

The more I think about this (default CLC version, and language version
restrictions), the more it looks like something that should be handled
by clang. The information is conceptually of the same kind as the set
of supported extensions.
My idea is to add default clc setting to clang/lib/Basic/Targets/, and
have a "Warning: CLC version incompatible with selected target" if the
invocation sets something that the target device does not support.
Clover can add "-Werror=clc-incompatible-version" to enforce build
failures.

I'm not sure if clang people will like the idea of having target
dependent default language version.

Jan

> 
> --Aaron
> 
> > 
> > Jan
> > 
> > > 
> > >        // clc.h requires that this macro be defined:
> > >        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thu, Aug 10, 2017 at 11:07 AM, Jan Vesely <jan.vesely@rutgers.edu> wrote:
> On Wed, 2017-08-09 at 22:36 -0500, Aaron Watry wrote:
>> On Fri, Aug 4, 2017 at 1:43 PM, Jan Vesely <jan.vesely@rutgers.edu> wrote:
>> > On Sun, 2017-07-30 at 20:26 -0500, Aaron Watry wrote:
>> > > Signed-off-by: Aaron Watry <awatry@gmail.com>
>> > > CC: Jan Vesely <jan.vesely@rutgers.edu>
>> > >
>> > > v2: base it on the device version
>> > > ---
>> > >  src/gallium/state_trackers/clover/llvm/invocation.cpp | 3 ++-
>> > >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > > index 63b2961752..443cd31e66 100644
>> > > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > > @@ -224,7 +224,8 @@ namespace {
>> > >        c.getPreprocessorOpts().Includes.push_back("clc/clc.h");
>> > >
>> > >        // Add definition for the OpenCL version
>> > > -      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=110");
>> > > +      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=" +
>> > > +              std::to_string(get_language_from_version_str(dev.device_version())));
>> >
>> > I don't think you can use the same parsing function here.
>> > __OPENCL_VERSION__ can go up to 2.2, while __OPENCL_C_VERSION__ is max
>> > 2.0
>>
>> Is that an issue here? I thought that the device's highest supported
>> OpenCL version was what was required here?
>
> The problem is that 'get_language_from_version_str' throws exception
> when the version is >2.0. It's OK for clc version, but device CL
> version can go above that.
> I don't think it's a big problem, a simple "TODO: consider higher
> versions" might be enough for now. it will take a while for clover to
> actually run into this.
>
>>
>> __OPENCL_VERSION__ is defined as "substitutes an integer reflecting
>> the version number of the OpenCL
>> supported by the OpenCL device", which in this case should map
>> directly to dev.device_version().
>>
>> __OPENCL_C_VERSION__ is already added by clang when the pre-processor
>> is initialized using the selected language version
>> (clang/FrontEnd/InitPreprocessor.cpp).
>
> The more I think about this (default CLC version, and language version
> restrictions), the more it looks like something that should be handled
> by clang. The information is conceptually of the same kind as the set
> of supported extensions.
> My idea is to add default clc setting to clang/lib/Basic/Targets/, and
> have a "Warning: CLC version incompatible with selected target" if the
> invocation sets something that the target device does not support.
> Clover can add "-Werror=clc-incompatible-version" to enforce build
> failures.
>
> I'm not sure if clang people will like the idea of having target
> dependent default language version.

We also would ideally also want to have some way of supporting the
existing LLVM/clang version as well, and then use whatever clang
provides whenever it's there.

Given that the list of supported extensions is determined by the
run-time in some cases (e.g. are popcount/printf implemented in libclc
when determining to expose 1.2), I'm not sure if the
version/capabilities are entirely something that can/should be
determined by clang.

--Aaron

>
> Jan
>
>>
>> --Aaron
>>
>> >
>> > Jan
>> >
>> > >
>> > >        // clc.h requires that this macro be defined:
>> > >        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev