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

[Jan Vesely]

On Mon, 2017-07-24 at 22:24 -0500, Aaron Watry wrote:
> On Mon, Jul 24, 2017 at 9:41 PM, Aaron Watry <awatry at gmail.com> wrote:
> > On Sat, Jul 22, 2017 at 2:59 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> > > On Fri, 2017-07-21 at 23:19 -0500, Aaron Watry wrote:
> > > > Base it on the active language version
> > > > 
> > > > Signed-off-by: Aaron Watry <awatry at gmail.com>
> > > > ---
> > > >  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 92d72e5b73..b562babf91 100644
> > > > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > > @@ -202,7 +202,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(c.getLangOpts().OpenCLVersion));
> > > 
> > > similar to previous patch. This should use device OCL version rather
> > > than language version. Moreover, I don't think value of this macro
> > > should be impacted by -cl-std= build parameter.
> > > __OPENCL_C_VERSION__ was added for to fill the gap of both above
> > > behaviours.
> > 
> > Fair enough.
> > 
> > I'll respin the series, taking this into account along with Pierre's
> > feedback. Having code in invocation.cpp that needs both device CL/CLC
> > version means we'll be plumbing either the device CL version and
> > device CLC version down into invocation.cpp, or possibly just passing
> > the itself device down.
> > 
> > I'm leaning towards passing the device down, and possibly adding a
> > numeric version of the device_version/device_clc_version fields to
> > prevent having to do as much string matching in what was patch 4.
> > 
> > Thoughts?
> 
> Also, there's a few other predefined macros that should be set, but
> aren't currently, and which are also based on device features:
> __IMAGE_SUPPORT__
> CL_DEVICE_MAX_GLOBAL_VARIABLE_SIZE (from 2.0)
> 
> I haven't actually looked yet, but we may also be missing the NULL
> macro (2.0+) and __kernel_exec macro. Given that they're not
> device-specific, we can circle back to that later.
> 
> Given the image/max_global defines that are also missing, I'm leaning
> fairly strongly towards just piping the device down into the functions
> in invocation.cpp and converting over the existing device members that
> are already sent through individually.

Hi,

thanks for looking into this.
I've also added Francisco to the discussion.

I haven't looked into all of the needed macros, but some of them sound
like it should be handled by clang. the current situation is bit of a
mess:
__OPENCL_C_VERSION__ is already handled by clang.
NULL is defined in libclc (clctypes.h)
__OPENCL_VERSION__ is probably easier to pass from clover.
CL_MAX_GLOBAL_VARIABLE_SIZE looks like host only macro.
__IMAGE_SUPPORT__ looks closer to __OPENCL_VERSION__ so I'd say it
could be passed by clover, but clang already handles the status of
cl_khr_3d_image_writes extension, so it might be a good idea to add
__IMAGE_SUPPORT__ there as well.


I haven't really considered all the possibilities. I'd say that
language specific macros should be handled by clang/libclc,
__OPENCL_VERSION__ and __IMAGE_SUPPORT__ can be passed by clover, just
to keep the authoritative information in one place.

if there is at least one macro that we'd like to provide from clover,
passing down device sounds like a reasonably future-proof approach.


Jan

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

-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- 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/20170725/c947cf65/attachment.sig>