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

[Jan Vesely]

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 at rutgers.edu> wrote:
> > On Sun, 2017-07-30 at 20:26 -0500, Aaron Watry wrote:
> > > Signed-off-by: Aaron Watry <awatry at gmail.com>
> > > CC: Jan Vesely <jan.vesely at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170810/0b65f632/attachment.sig>