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

[Aaron Watry]

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.

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