[4/5] clover/llvm: Add get_[cl|language]_version, validation and some helpers

Submitted by Aaron Watry on March 1, 2018, 7:39 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Aaron Watry March 1, 2018, 7:39 p.m.
Used to calculate the default CLC language version based on the --cl-std in build args
and the device capabilities.

According to section 5.8.4.5 of the 2.0 spec, the CL C version is chosen by:
 1) If you have -cl-std=CL1.1+ use the version specified
 2) If not, use the highest 1.x version that the device supports

Curiously, there is no valid value for -cl-std=CL1.0

Validates requested cl-std against device_clc_version

Signed-off-by: Aaron Watry <awatry@gmail.com>
Cc: Pierre Moreau <pierre.morrow@free.fr>

v5: (Aaron) Use a collection of cl versions instead of switch cases
    Consolidates the string, numeric version, and clc langstandard::kind

v4: (Pierre) Split get_language_version addition and use into separate patches
    Squash patches that add the helpers and validate the language standard

v3: Change device_version to device_clc_version

v2: (Pierre) Move create_compiler_instance changes to correct patch
    to prevent temporary build breakage.
    Convert version_str into unsigned and use it to find language version
    Add build_error for unknown language version string
    Whitespace fixes
---
 .../state_trackers/clover/llvm/invocation.cpp      | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)

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 1924c0317f..8d76f203de 100644
--- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
+++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
@@ -63,6 +63,23 @@  using ::llvm::Module;
 using ::llvm::raw_string_ostream;
 
 namespace {
+
+   struct cl_version {
+      std::string version_str; //CL Version
+      unsigned version_number; //Numeric CL Version
+      clang::LangStandard::Kind clc_lang_standard; //CLC standard of this version
+   };
+
+   static const unsigned ANY_VERSION = 999;
+   cl_version const cl_versions[] = {
+      { "1.0", 100, clang::LangStandard::lang_opencl10},
+      { "1.1", 110, clang::LangStandard::lang_opencl11},
+      { "1.2", 120, clang::LangStandard::lang_opencl12},
+      { "2.0", 200, clang::LangStandard::lang_opencl20},
+      { "2.1", 210, clang::LangStandard::lang_unspecified}, //2.1 doesn't exist
+      { "2.2", 220, clang::LangStandard::lang_unspecified}, //2.2 doesn't exist
+   };
+
    void
    init_targets() {
       static bool targets_initialized = false;
@@ -93,6 +110,52 @@  namespace {
       return ctx;
    }
 
+   struct cl_version
+   get_cl_version(const std::string &version_str,
+                  unsigned max = ANY_VERSION) {
+      for (struct cl_version version : cl_versions) {
+         if (version.version_number == max || version.version_str == version_str) {
+            return version;
+         }
+      }
+      throw build_error("Unknown/Unsupported language version");
+   }
+
+   clang::LangStandard::Kind
+   get_lang_standard_from_version_str(const std::string &version_str,
+                                      bool is_build_opt = false) {
+       /**
+       * Per CL 2.0 spec, section 5.8.4.5:
+       * If it's an option, use the value directly.
+       * If it's a device version, clamp to max 1.x version, a.k.a. 1.2
+       */
+      struct cl_version version = get_cl_version(version_str,
+                                      is_build_opt ? ANY_VERSION : 120);
+      return version.clc_lang_standard;
+   }
+
+   clang::LangStandard::Kind
+   get_language_version(const std::vector<std::string> &opts,
+                        const std::string &device_version) {
+
+      const std::string search = "-cl-std=CL";
+
+      for(auto opt: opts){
+         auto pos = opt.find(search);
+         if (pos == 0){
+            auto ver = opt.substr(pos+search.size());
+            auto device_ver = get_cl_version(device_version);
+            auto requested = get_cl_version(ver);
+            if (requested.version_number > device_ver.version_number) {
+               throw build_error();
+            }
+            return get_lang_standard_from_version_str(ver, true);
+         }
+      }
+
+      return get_lang_standard_from_version_str(device_version);
+   }
+
    std::unique_ptr<clang::CompilerInstance>
    create_compiler_instance(const device &dev,
                             const std::vector<std::string> &opts,

Comments

On 2018-03-01 — 13:39, Aaron Watry wrote:
> Used to calculate the default CLC language version based on the --cl-std in build args
> and the device capabilities.
> 
> According to section 5.8.4.5 of the 2.0 spec, the CL C version is chosen by:
>  1) If you have -cl-std=CL1.1+ use the version specified
>  2) If not, use the highest 1.x version that the device supports
> 
> Curiously, there is no valid value for -cl-std=CL1.0
> 
> Validates requested cl-std against device_clc_version
> 
> Signed-off-by: Aaron Watry <awatry@gmail.com>
> Cc: Pierre Moreau <pierre.morrow@free.fr>
> 
> v5: (Aaron) Use a collection of cl versions instead of switch cases
>     Consolidates the string, numeric version, and clc langstandard::kind
> 
> v4: (Pierre) Split get_language_version addition and use into separate patches
>     Squash patches that add the helpers and validate the language standard
> 
> v3: Change device_version to device_clc_version
> 
> v2: (Pierre) Move create_compiler_instance changes to correct patch
>     to prevent temporary build breakage.
>     Convert version_str into unsigned and use it to find language version
>     Add build_error for unknown language version string
>     Whitespace fixes
> ---
>  .../state_trackers/clover/llvm/invocation.cpp      | 63 ++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index 1924c0317f..8d76f203de 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -63,6 +63,23 @@ using ::llvm::Module;
>  using ::llvm::raw_string_ostream;
>  
>  namespace {
> +
> +   struct cl_version {

I would rename everything that uses *cl_version* to *clc_version*, as they are
all about the OpenCL C language version, rather than the OpenCL API version.

> +      std::string version_str; //CL Version

Minor change, but a space could be added between the comment token and the
comment itself (same for the other comments further down).
I would go “OpenCL C” instead of just “CL” in the comment.

> +      unsigned version_number; //Numeric CL Version
> +      clang::LangStandard::Kind clc_lang_standard; //CLC standard of this version

Similarly here.

> +   };
> +
> +   static const unsigned ANY_VERSION = 999;
> +   cl_version const cl_versions[] = {

Please place “const” before the type, for consistency.

> +      { "1.0", 100, clang::LangStandard::lang_opencl10},
> +      { "1.1", 110, clang::LangStandard::lang_opencl11},
> +      { "1.2", 120, clang::LangStandard::lang_opencl12},
> +      { "2.0", 200, clang::LangStandard::lang_opencl20},
> +      { "2.1", 210, clang::LangStandard::lang_unspecified}, //2.1 doesn't exist
> +      { "2.2", 220, clang::LangStandard::lang_unspecified}, //2.2 doesn't exist

You should remove 2.1 and 2.2, as those versions of OpenCL C do not exist, and
“CL2.1” or “CL2.2” are not valid values to “-cl-std”.

> +   };
> +
>     void
>     init_targets() {
>        static bool targets_initialized = false;
> @@ -93,6 +110,52 @@ namespace {
>        return ctx;
>     }
>  
> +   struct cl_version
> +   get_cl_version(const std::string &version_str,
> +                  unsigned max = ANY_VERSION) {
> +      for (struct cl_version version : cl_versions) {

You could take a constant reference here.

> +         if (version.version_number == max || version.version_str == version_str) {
> +            return version;
> +         }
> +      }
> +      throw build_error("Unknown/Unsupported language version");
> +   }
> +
> +   clang::LangStandard::Kind
> +   get_lang_standard_from_version_str(const std::string &version_str,
> +                                      bool is_build_opt = false) {
> +       /**
> +       * Per CL 2.0 spec, section 5.8.4.5:
> +       * If it's an option, use the value directly.
> +       * If it's a device version, clamp to max 1.x version, a.k.a. 1.2
> +       */
> +      struct cl_version version = get_cl_version(version_str,
> +                                      is_build_opt ? ANY_VERSION : 120);
> +      return version.clc_lang_standard;
> +   }
> +
> +   clang::LangStandard::Kind
> +   get_language_version(const std::vector<std::string> &opts,
> +                        const std::string &device_version) {
> +
> +      const std::string search = "-cl-std=CL";
> +
> +      for(auto opt: opts){

Missing spaces after “for” and before ‘{’.

> +         auto pos = opt.find(search);
> +         if (pos == 0){
> +            auto ver = opt.substr(pos+search.size());

You could have spaces around the ‘+’.
And variables here could be marked as constant.

> +            auto device_ver = get_cl_version(device_version);
> +            auto requested = get_cl_version(ver);
> +            if (requested.version_number > device_ver.version_number) {
> +               throw build_error();
> +            }
> +            return get_lang_standard_from_version_str(ver, true);
> +         }
> +      }
> +
> +      return get_lang_standard_from_version_str(device_version);
> +   }
> +
>     std::unique_ptr<clang::CompilerInstance>
>     create_compiler_instance(const device &dev,
>                              const std::vector<std::string> &opts,
> -- 
> 2.14.1
>
On 2018-03-01 — 22:43, Pierre Moreau wrote:
> On 2018-03-01 — 13:39, Aaron Watry wrote:
> > Used to calculate the default CLC language version based on the --cl-std in build args
> > and the device capabilities.
> > 
> > According to section 5.8.4.5 of the 2.0 spec, the CL C version is chosen by:
> >  1) If you have -cl-std=CL1.1+ use the version specified
> >  2) If not, use the highest 1.x version that the device supports
> > 
> > Curiously, there is no valid value for -cl-std=CL1.0
> > 
> > Validates requested cl-std against device_clc_version
> > 
> > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > Cc: Pierre Moreau <pierre.morrow@free.fr>
> > 
> > v5: (Aaron) Use a collection of cl versions instead of switch cases
> >     Consolidates the string, numeric version, and clc langstandard::kind
> > 
> > v4: (Pierre) Split get_language_version addition and use into separate patches
> >     Squash patches that add the helpers and validate the language standard
> > 
> > v3: Change device_version to device_clc_version
> > 
> > v2: (Pierre) Move create_compiler_instance changes to correct patch
> >     to prevent temporary build breakage.
> >     Convert version_str into unsigned and use it to find language version
> >     Add build_error for unknown language version string
> >     Whitespace fixes
> > ---
> >  .../state_trackers/clover/llvm/invocation.cpp      | 63 ++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> > 
> > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > index 1924c0317f..8d76f203de 100644
> > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > @@ -63,6 +63,23 @@ using ::llvm::Module;
> >  using ::llvm::raw_string_ostream;
> >  
> >  namespace {
> > +
> > +   struct cl_version {
> 
> I would rename everything that uses *cl_version* to *clc_version*, as they are
> all about the OpenCL C language version, rather than the OpenCL API version.

Hum, I just saw you uses it for converting the device OpenCL API version as
well. I need to have another look at this patch.

> 
> > +      std::string version_str; //CL Version
> 
> Minor change, but a space could be added between the comment token and the
> comment itself (same for the other comments further down).
> I would go “OpenCL C” instead of just “CL” in the comment.
> 
> > +      unsigned version_number; //Numeric CL Version
> > +      clang::LangStandard::Kind clc_lang_standard; //CLC standard of this version
> 
> Similarly here.
> 
> > +   };
> > +
> > +   static const unsigned ANY_VERSION = 999;
> > +   cl_version const cl_versions[] = {
> 
> Please place “const” before the type, for consistency.
> 
> > +      { "1.0", 100, clang::LangStandard::lang_opencl10},
> > +      { "1.1", 110, clang::LangStandard::lang_opencl11},
> > +      { "1.2", 120, clang::LangStandard::lang_opencl12},
> > +      { "2.0", 200, clang::LangStandard::lang_opencl20},
> > +      { "2.1", 210, clang::LangStandard::lang_unspecified}, //2.1 doesn't exist
> > +      { "2.2", 220, clang::LangStandard::lang_unspecified}, //2.2 doesn't exist
> 
> You should remove 2.1 and 2.2, as those versions of OpenCL C do not exist, and
> “CL2.1” or “CL2.2” are not valid values to “-cl-std”.
> 
> > +   };
> > +
> >     void
> >     init_targets() {
> >        static bool targets_initialized = false;
> > @@ -93,6 +110,52 @@ namespace {
> >        return ctx;
> >     }
> >  
> > +   struct cl_version
> > +   get_cl_version(const std::string &version_str,
> > +                  unsigned max = ANY_VERSION) {
> > +      for (struct cl_version version : cl_versions) {
> 
> You could take a constant reference here.
> 
> > +         if (version.version_number == max || version.version_str == version_str) {
> > +            return version;
> > +         }
> > +      }
> > +      throw build_error("Unknown/Unsupported language version");
> > +   }
> > +
> > +   clang::LangStandard::Kind
> > +   get_lang_standard_from_version_str(const std::string &version_str,
> > +                                      bool is_build_opt = false) {
> > +       /**
> > +       * Per CL 2.0 spec, section 5.8.4.5:
> > +       * If it's an option, use the value directly.
> > +       * If it's a device version, clamp to max 1.x version, a.k.a. 1.2
> > +       */
> > +      struct cl_version version = get_cl_version(version_str,
> > +                                      is_build_opt ? ANY_VERSION : 120);
> > +      return version.clc_lang_standard;
> > +   }
> > +
> > +   clang::LangStandard::Kind
> > +   get_language_version(const std::vector<std::string> &opts,
> > +                        const std::string &device_version) {
> > +
> > +      const std::string search = "-cl-std=CL";
> > +
> > +      for(auto opt: opts){
> 
> Missing spaces after “for” and before ‘{’.
> 
> > +         auto pos = opt.find(search);
> > +         if (pos == 0){
> > +            auto ver = opt.substr(pos+search.size());
> 
> You could have spaces around the ‘+’.
> And variables here could be marked as constant.
> 
> > +            auto device_ver = get_cl_version(device_version);
> > +            auto requested = get_cl_version(ver);
> > +            if (requested.version_number > device_ver.version_number) {
> > +               throw build_error();
> > +            }
> > +            return get_lang_standard_from_version_str(ver, true);
> > +         }
> > +      }
> > +
> > +      return get_lang_standard_from_version_str(device_version);
> > +   }
> > +
> >     std::unique_ptr<clang::CompilerInstance>
> >     create_compiler_instance(const device &dev,
> >                              const std::vector<std::string> &opts,
> > -- 
> > 2.14.1
> > 



> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thu, Mar 1, 2018 at 3:43 PM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> On 2018-03-01 — 13:39, Aaron Watry wrote:
>> Used to calculate the default CLC language version based on the --cl-std in build args
>> and the device capabilities.
>>
>> According to section 5.8.4.5 of the 2.0 spec, the CL C version is chosen by:
>>  1) If you have -cl-std=CL1.1+ use the version specified
>>  2) If not, use the highest 1.x version that the device supports
>>
>> Curiously, there is no valid value for -cl-std=CL1.0
>>
>> Validates requested cl-std against device_clc_version
>>
>> Signed-off-by: Aaron Watry <awatry@gmail.com>
>> Cc: Pierre Moreau <pierre.morrow@free.fr>
>>
>> v5: (Aaron) Use a collection of cl versions instead of switch cases
>>     Consolidates the string, numeric version, and clc langstandard::kind
>>
>> v4: (Pierre) Split get_language_version addition and use into separate patches
>>     Squash patches that add the helpers and validate the language standard
>>
>> v3: Change device_version to device_clc_version
>>
>> v2: (Pierre) Move create_compiler_instance changes to correct patch
>>     to prevent temporary build breakage.
>>     Convert version_str into unsigned and use it to find language version
>>     Add build_error for unknown language version string
>>     Whitespace fixes
>> ---
>>  .../state_trackers/clover/llvm/invocation.cpp      | 63 ++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>>
>> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> index 1924c0317f..8d76f203de 100644
>> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> @@ -63,6 +63,23 @@ using ::llvm::Module;
>>  using ::llvm::raw_string_ostream;
>>
>>  namespace {
>> +
>> +   struct cl_version {
>
> I would rename everything that uses *cl_version* to *clc_version*, as they are
> all about the OpenCL C language version, rather than the OpenCL API version.
>
Yup, I use this set of declarations again in the last patch of the
series for determining CL version
when setting __OPENCL_VERSION__, hence the name I chose.

I'll address the other comments you had in this file which were not
related to the cl/clc confusion for now.

>> +      std::string version_str; //CL Version
>
> Minor change, but a space could be added between the comment token and the
> comment itself (same for the other comments further down).

I added a space here and on the next two comments.

In order to stay <= 80 chars, I changed the last one to:
// Lang standard for version

It was at 82 characters before anyway.

> I would go “OpenCL C” instead of just “CL” in the comment.
>
>> +      unsigned version_number; //Numeric CL Version
>> +      clang::LangStandard::Kind clc_lang_standard; //CLC standard of this version
>
> Similarly here.
>
>> +   };
>> +
>> +   static const unsigned ANY_VERSION = 999;
>> +   cl_version const cl_versions[] = {
>
> Please place “const” before the type, for consistency.

Done.

>
>> +      { "1.0", 100, clang::LangStandard::lang_opencl10},
>> +      { "1.1", 110, clang::LangStandard::lang_opencl11},
>> +      { "1.2", 120, clang::LangStandard::lang_opencl12},
>> +      { "2.0", 200, clang::LangStandard::lang_opencl20},
>> +      { "2.1", 210, clang::LangStandard::lang_unspecified}, //2.1 doesn't exist
>> +      { "2.2", 220, clang::LangStandard::lang_unspecified}, //2.2 doesn't exist
>
> You should remove 2.1 and 2.2, as those versions of OpenCL C do not exist, and
> “CL2.1” or “CL2.2” are not valid values to “-cl-std”.
>
>> +   };
>> +
>>     void
>>     init_targets() {
>>        static bool targets_initialized = false;
>> @@ -93,6 +110,52 @@ namespace {
>>        return ctx;
>>     }
>>
>> +   struct cl_version
>> +   get_cl_version(const std::string &version_str,
>> +                  unsigned max = ANY_VERSION) {
>> +      for (struct cl_version version : cl_versions) {
>
> You could take a constant reference here.

Done.

I also make the function return type const as well.


>
>> +         if (version.version_number == max || version.version_str == version_str) {
>> +            return version;
>> +         }
>> +      }
>> +      throw build_error("Unknown/Unsupported language version");
>> +   }
>> +
>> +   clang::LangStandard::Kind
>> +   get_lang_standard_from_version_str(const std::string &version_str,
>> +                                      bool is_build_opt = false) {
>> +       /**
>> +       * Per CL 2.0 spec, section 5.8.4.5:
>> +       * If it's an option, use the value directly.
>> +       * If it's a device version, clamp to max 1.x version, a.k.a. 1.2
>> +       */
>> +      struct cl_version version = get_cl_version(version_str,

I made this const as well.

>> +                                      is_build_opt ? ANY_VERSION : 120);
>> +      return version.clc_lang_standard;
>> +   }
>> +
>> +   clang::LangStandard::Kind
>> +   get_language_version(const std::vector<std::string> &opts,
>> +                        const std::string &device_version) {
>> +
>> +      const std::string search = "-cl-std=CL";
>> +
>> +      for(auto opt: opts){
>
> Missing spaces after “for” and before ‘{’.

Fixed locally.
>
>> +         auto pos = opt.find(search);
>> +         if (pos == 0){
>> +            auto ver = opt.substr(pos+search.size());
>
> You could have spaces around the ‘+’.
> And variables here could be marked as constant.

Fixed.

I'll send an updated patch 4 in a minute so you can see the new version.

--Aaron

>
>> +            auto device_ver = get_cl_version(device_version);
>> +            auto requested = get_cl_version(ver);
>> +            if (requested.version_number > device_ver.version_number) {
>> +               throw build_error();
>> +            }
>> +            return get_lang_standard_from_version_str(ver, true);
>> +         }
>> +      }
>> +
>> +      return get_lang_standard_from_version_str(device_version);
>> +   }
>> +
>>     std::unique_ptr<clang::CompilerInstance>
>>     create_compiler_instance(const device &dev,
>>                              const std::vector<std::string> &opts,
>> --
>> 2.14.1
>>