[Mesa-dev] [PATCH 4/5] clover/llvm: Add get_[cl|language]_version, validation and some helpers
Aaron Watry
awatry at gmail.com
Fri Mar 2 02:01:06 UTC 2018
On Thu, Mar 1, 2018 at 3:43 PM, Pierre Moreau <pierre.morrow at 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 at gmail.com>
>> Cc: Pierre Moreau <pierre.morrow at 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
>>
More information about the mesa-dev
mailing list