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

Submitted by Aaron Watry on July 22, 2017, 4:19 a.m.

Details

Message ID 20170722041951.14304-6-awatry@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Aaron Watry July 22, 2017, 4:19 a.m.
Base it on the active language version

Signed-off-by: Aaron Watry <awatry@gmail.com>
---
 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 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));
 
       // clc.h requires that this macro be defined:
       c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");

Comments

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

Jan

>  
>        // clc.h requires that this macro be defined:
>        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
On Sat, Jul 22, 2017 at 2:59 PM, Jan Vesely <jan.vesely@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@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?

--Aaron

>
> Jan
>
>>
>>        // clc.h requires that this macro be defined:
>>        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
>
> --
> Jan Vesely <jan.vesely@rutgers.edu>
On Mon, Jul 24, 2017 at 9:41 PM, Aaron Watry <awatry@gmail.com> wrote:
> On Sat, Jul 22, 2017 at 2:59 PM, Jan Vesely <jan.vesely@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@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.

--Aaron

>
> --Aaron
>
>>
>> Jan
>>
>>>
>>>        // clc.h requires that this macro be defined:
>>>        c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
>>
>> --
>> Jan Vesely <jan.vesely@rutgers.edu>
On Mon, 2017-07-24 at 22:24 -0500, Aaron Watry wrote:
> On Mon, Jul 24, 2017 at 9:41 PM, Aaron Watry <awatry@gmail.com> wrote:
> > On Sat, Jul 22, 2017 at 2:59 PM, Jan Vesely <jan.vesely@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@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@rutgers.edu>
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev