On 10/07/2016 12:05 PM, Serge Martin wrote:
> On Thursday 06 October 2016 16:26:21 Vedran Miletić wrote:
>> CLOVER_CL_VERSION_OVERRIDE allows overriding default OpenCL version
>> supported by Clover, analogous to MESA_GL_VERSION_OVERRIDE for OpenGL.
>> CLOVER_CL_C_VERSION_OVERRIDE allows overridng default OpenCL C version.
> 
> Thanks.
> I've made some comments bellow
> 
>> ---
>>  docs/envvars.html                                     | 12 ++++++++++++
>>  src/gallium/state_trackers/clover/api/device.cpp      |  4 ++--
>>  src/gallium/state_trackers/clover/api/platform.cpp    |  4 ++--
>>  src/gallium/state_trackers/clover/core/device.cpp     | 19
>> +++++++++++++++++++ src/gallium/state_trackers/clover/core/device.hpp     |
>>  4 ++++
>>  src/gallium/state_trackers/clover/core/platform.cpp   |  9 +++++++++
>>  src/gallium/state_trackers/clover/core/platform.hpp   |  3 +++
>>  src/gallium/state_trackers/clover/core/program.cpp    |  4 +++-
>>  src/gallium/state_trackers/clover/llvm/invocation.cpp | 18
>> ++++++++++++++---- src/gallium/state_trackers/clover/llvm/invocation.hpp | 
>> 1 +
>>  src/gallium/state_trackers/clover/llvm/util.hpp       |  4 ++--
>>  11 files changed, 71 insertions(+), 11 deletions(-)
>>
>> diff --git a/docs/envvars.html b/docs/envvars.html
>> index cf57ca5..f76827b 100644
>> --- a/docs/envvars.html
>> +++ b/docs/envvars.html
>> @@ -235,6 +235,18 @@ Setting to "tgsi", for example, will print all the TGSI
>> shaders. See src/mesa/state_tracker/st_debug.c for other options.
>>  </ul>
>>
>> +<h3>Clover state tracker environment variables</h3>
>> +
>> +<ul>
>> +<li>CLOVER_CL_VERSION_OVERRIDE - allows overriding OpenCL version returned
>> by +    clGetPlatformInfo(CL_PLATFORM_VERSION) and
>> +    clGetDeviceInfo(CL_DEVICE_VERSION). Note that the setting sets the
>> version +    of the platform and all the devices to the same value.
>> +<li>CLOVER_CL_C_VERSION_OVERRIDE - allows overriding OpenCL C version
>> reported +    by clGetDeviceInfo(CL_DEVICE_OPENCL_C_VERSION) and the value
>> of the +    __OPENCL_VERSION__ macro in the OpenCL compiler.
> 
> I don't think we need CLOVER_CL_C_VERSION_OVERRIDE. 
> CLOVER_CL_VERSION_OVERRIDE 
> should be enough.
> 

I believe we do because with OpenCL 2.1+ the version of OpenCL C is
still 2.0.

>> +</ul>
>> +
>>  <h3>Softpipe driver environment variables</h3>
>>  <ul>
>>  <li>SOFTPIPE_DUMP_FS - if set, the softpipe driver will print fragment
>> shaders diff --git a/src/gallium/state_trackers/clover/api/device.cpp
>> b/src/gallium/state_trackers/clover/api/device.cpp index f7bd61b..e23de7a
>> 100644
>> --- a/src/gallium/state_trackers/clover/api/device.cpp
>> +++ b/src/gallium/state_trackers/clover/api/device.cpp
>> @@ -301,7 +301,7 @@ clGetDeviceInfo(cl_device_id d_dev, cl_device_info
>> param, break;
>>
>>     case CL_DEVICE_VERSION:
>> -      buf.as_string() = "OpenCL 1.1 Mesa " PACKAGE_VERSION
>> +      buf.as_string() = "OpenCL " + dev.opencl_version() + " Mesa "
>> PACKAGE_VERSION #ifdef MESA_GIT_SHA1
>>                          " (" MESA_GIT_SHA1 ")"
>>  #endif
>> @@ -355,7 +355,7 @@ clGetDeviceInfo(cl_device_id d_dev, cl_device_info
>> param, break;
>>
>>     case CL_DEVICE_OPENCL_C_VERSION:
>> -      buf.as_string() = "OpenCL C 1.1 ";
>> +      buf.as_string() = "OpenCL C " + dev.opencl_c_version() + " ";
>>        break;
>>
>>     case CL_DEVICE_PARENT_DEVICE:
>> diff --git a/src/gallium/state_trackers/clover/api/platform.cpp
>> b/src/gallium/state_trackers/clover/api/platform.cpp index b1b1fdf..f344ec8
>> 100644
>> --- a/src/gallium/state_trackers/clover/api/platform.cpp
>> +++ b/src/gallium/state_trackers/clover/api/platform.cpp
>> @@ -50,7 +50,7 @@ clover::GetPlatformInfo(cl_platform_id d_platform,
>> cl_platform_info param, size_t size, void *r_buf, size_t *r_size) try {
>> property_buffer buf { r_buf, size, r_size };
>>
>> -   obj(d_platform);
>> +   auto &platform = obj(d_platform);
>>
>>     switch (param) {
>>     case CL_PLATFORM_PROFILE:
>> @@ -58,7 +58,7 @@ clover::GetPlatformInfo(cl_platform_id d_platform,
>> cl_platform_info param, break;
>>
>>     case CL_PLATFORM_VERSION:
>> -      buf.as_string() = "OpenCL 1.1 Mesa " PACKAGE_VERSION
>> +      buf.as_string() = "OpenCL " + platform.opencl_version() + " Mesa "
> 
> 
> you don't need to add an opencl_version() func to core/plateform,
> it won't be used except here.
> I prefer if we have an util function that would be used here, in device.cpp 
> and invocation.cpp because for the moment this is still a global value.
> 
> 
>> PACKAGE_VERSION #ifdef MESA_GIT_SHA1
>>                          " (" MESA_GIT_SHA1 ")"
>>  #endif
>> diff --git a/src/gallium/state_trackers/clover/core/device.cpp
>> b/src/gallium/state_trackers/clover/core/device.cpp index 8825f99..fce6fb3
>> 100644
>> --- a/src/gallium/state_trackers/clover/core/device.cpp
>> +++ b/src/gallium/state_trackers/clover/core/device.cpp
>> @@ -24,6 +24,7 @@
>>  #include "core/platform.hpp"
>>  #include "pipe/p_screen.h"
>>  #include "pipe/p_state.h"
>> +#include "util/u_debug.h"
>>
>>  using namespace clover;
>>
>> @@ -48,6 +49,14 @@ device::device(clover::platform &platform,
>> pipe_loader_device *ldev) : pipe->destroy(pipe);
>>        throw error(CL_INVALID_DEVICE);
>>     }
>> +
>> +   const std::string cl_version_override =
>> +                             debug_get_option("CLOVER_CL_VERSION_OVERRIDE",
>> ""); +   ocl_version = !cl_version_override.empty() ? cl_version_override :
>> "1.1";
> 
> This is what the default value of debug_get_option is for, this is redundant. 
> You just have to pass "1.1".
> 
> Also, if we have a util function that statistically keep the value the extra 
> device variable and funcs are not needed.
> We may need it if we go for CL 2.0 latter with some devices only advertising 
> CL 1.2 because of unsupported feature, but not for the moment.
> 

Thanks for the default value comment, did not know it. Will fix it. I do
agree with a util function that would return "1.1" until we choose to
bump it. However I would prefer to have function per-device because we
will eventually need to do it like that anyway.

>> +
>> +   const std::string clc_version_override =
>> +                            debug_get_option("CLOVER_CLC_VERSION_OVERRIDE",
>> ""); +   oclc_version = !clc_version_override.empty() ?
>> clc_version_override : "1.1"; }
>>
>>  device::~device() {
>> @@ -209,6 +218,16 @@ device::vendor_name() const {
>>     return pipe->get_device_vendor(pipe);
>>  }
>>
>> +std::string
>> +device::opencl_version() const {
>> +    return ocl_version;
>> +}
>> +
>> +std::string
>> +device::opencl_c_version() const {
>> +    return oclc_version;
>> +}
>> +
>>  enum pipe_shader_ir
>>  device::ir_format() const {
>>     return (enum pipe_shader_ir) pipe->get_shader_param(
>> diff --git a/src/gallium/state_trackers/clover/core/device.hpp
>> b/src/gallium/state_trackers/clover/core/device.hpp index 6cf6c7f..b71cafd
>> 100644
>> --- a/src/gallium/state_trackers/clover/core/device.hpp
>> +++ b/src/gallium/state_trackers/clover/core/device.hpp
>> @@ -71,6 +71,8 @@ namespace clover {
>>        cl_uint address_bits() const;
>>        std::string device_name() const;
>>        std::string vendor_name() const;
>> +      std::string opencl_version() const;
>> +      std::string opencl_c_version() const;
>>        enum pipe_shader_ir ir_format() const;
>>        std::string ir_target() const;
>>        enum pipe_endian endianness() const;
>> @@ -86,6 +88,8 @@ namespace clover {
>>     private:
>>        pipe_screen *pipe;
>>        pipe_loader_device *ldev;
>> +      std::string ocl_version;
>> +      std::string oclc_version;
>>     };
>>  }
>>
>> diff --git a/src/gallium/state_trackers/clover/core/platform.cpp
>> b/src/gallium/state_trackers/clover/core/platform.cpp index
>> 489e8dc..c5d47f0 100644
>> --- a/src/gallium/state_trackers/clover/core/platform.cpp
>> +++ b/src/gallium/state_trackers/clover/core/platform.cpp
>> @@ -21,6 +21,7 @@
>>  //
>>
>>  #include "core/platform.hpp"
>> +#include "util/u_debug.h"
>>
>>  using namespace clover;
>>
>> @@ -38,4 +39,12 @@ platform::platform() : adaptor_range(evals(), devs) {
>>           pipe_loader_release(&ldev, 1);
>>        }
>>     }
>> +
>> +   const std::string cl_version_override =
>> +                             debug_get_option("CLOVER_CL_VERSION_OVERRIDE",
>> ""); +   ocl_version = !cl_version_override.empty() ? cl_version_override :
>> "1.1"; +}
>> +
>> +std::string platform::opencl_version() {
>> +    return ocl_version;
>>  }
>> diff --git a/src/gallium/state_trackers/clover/core/platform.hpp
>> b/src/gallium/state_trackers/clover/core/platform.hpp index
>> e849645..b283044 100644
>> --- a/src/gallium/state_trackers/clover/core/platform.hpp
>> +++ b/src/gallium/state_trackers/clover/core/platform.hpp
>> @@ -40,8 +40,11 @@ namespace clover {
>>        platform &
>>        operator=(const platform &platform) = delete;
>>
>> +      std::string opencl_version();
>> +
>>     protected:
>>        std::vector<intrusive_ref<device>> devs;
>> +      std::string ocl_version;
>>     };
>>  }
>>
>> diff --git a/src/gallium/state_trackers/clover/core/program.cpp
>> b/src/gallium/state_trackers/clover/core/program.cpp index 79ac851..0cb5e77
>> 100644
>> --- a/src/gallium/state_trackers/clover/core/program.cpp
>> +++ b/src/gallium/state_trackers/clover/core/program.cpp
>> @@ -54,7 +54,9 @@ program::compile(const ref_vector<device> &devs, const
>> std::string &opts, const module m = (dev.ir_format() == PIPE_SHADER_IR_TGSI
>> ? tgsi::compile_program(_source, log) : llvm::compile_program(_source,
>> headers, -                                                   
>> dev.ir_target(), opts, log)); +                                            
>>        dev.ir_target(), +                                                  
>>  dev.opencl_c_version(), +                                                 
>>   opts, log));
>>              _builds[&dev] = { m, opts, log };
>>           } catch (...) {
>>              _builds[&dev] = { module(), opts, log };
>> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> b/src/gallium/state_trackers/clover/llvm/invocation.cpp index
>> b5e8b52..2a47acf 100644
>> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> @@ -24,6 +24,8 @@
>>  // OTHER DEALINGS IN THE SOFTWARE.
>>  //
>>
>> +#include <cassert>
>> +
>>  #include <llvm/IR/DiagnosticPrinter.h>
>>  #include <llvm/IR/DiagnosticInfo.h>
>>  #include <llvm/IR/LLVMContext.h>
>> @@ -139,7 +141,8 @@ namespace {
>>     compile(LLVMContext &ctx, clang::CompilerInstance &c,
>>             const std::string &name, const std::string &source,
>>             const header_map &headers, const std::string &target,
>> -           const std::string &opts, std::string &r_log) {
>> +           const std::string &opencl_version, const std::string &opts,
>> +           std::string &r_log) {
> 
> The version arg is not needed for the moment, all device will advertise the 
> same version for the moment. Please use an util func
> 

Same comment as above.

>>        c.getFrontendOpts().ProgramAction = clang::frontend::EmitLLVMOnly;
>>        c.getHeaderSearchOpts().UseBuiltinIncludes = true;
>>        c.getHeaderSearchOpts().UseStandardSystemIncludes = true;
>> @@ -154,7 +157,13 @@ namespace {
>>        c.getPreprocessorOpts().Includes.push_back("clc/clc.h");
>>
>>        // Add definition for the OpenCL version
>> -      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=110");
>> +      auto ocl_version_major_minor = tokenize(opencl_version, '.');
>> +      assert(ocl_version_major_minor.size() == 2);
> 
> I'm not fan of doing assert when we have a way to report error on user.
> If the version is wrong, stop the compilation with CL_INVALID_BUILD_OPTION or 
> some things like that and fill the log.
> 

Good idea.

>> +      int ocl_version_major = stoi(ocl_version_major_minor[0]);
>> +      int ocl_version_minor = stoi(ocl_version_major_minor[1]);
>> +      int ocl_version_number = ocl_version_major * 100 + ocl_version_minor
>> * 10; +      c.getPreprocessorOpts().addMacroDef("__OPENCL_VERSION__=" + + 
>>                                        
>> std::to_string(ocl_version_number));
> 
> so this version[0] + version[2] + "0". I think we can go without stoi.
> 

Assuming that OpenCL never reaches double-digit major version number. I
prefer proper "split by the dot".

> Also you need to change lang_opencl11 that is passed as default unless adding 
> cl-std=XX (when XX > 1.1) on command line override it.
> 

Indeed, also noted by Jan.

> Serge
> 
>>
>>        // clc.h requires that this macro be defined:
>>       
>> c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
>> @@ -197,6 +206,7 @@ module
>>  clover::llvm::compile_program(const std::string &source,
>>                                const header_map &headers,
>>                                const std::string &target,
>> +                              const std::string &opencl_version,
>>                                const std::string &opts,
>>                                std::string &r_log) {
>>     if (has_flag(debug::clc))
>> @@ -205,8 +215,8 @@ clover::llvm::compile_program(const std::string &source,
>> auto ctx = create_context(r_log);
>>     auto c = create_compiler_instance(target, tokenize(opts + " input.cl"),
>>                                       r_log);
>> -   auto mod = compile(*ctx, *c, "input.cl", source, headers, target, opts,
>> -                      r_log);
>> +   auto mod = compile(*ctx, *c, "input.cl", source, headers, target,
>> +                      opencl_version, opts, r_log);
>>
>>     if (has_flag(debug::llvm))
>>        debug::log(".ll", print_module_bitcode(*mod));
>> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.hpp
>> b/src/gallium/state_trackers/clover/llvm/invocation.hpp index
>> 5b3530c..2493cbf 100644
>> --- a/src/gallium/state_trackers/clover/llvm/invocation.hpp
>> +++ b/src/gallium/state_trackers/clover/llvm/invocation.hpp
>> @@ -33,6 +33,7 @@ namespace clover {
>>        module compile_program(const std::string &source,
>>                               const header_map &headers,
>>                               const std::string &target,
>> +                             const std::string &opencl_version,
>>                               const std::string &opts,
>>                               std::string &r_log);
>>
>> diff --git a/src/gallium/state_trackers/clover/llvm/util.hpp
>> b/src/gallium/state_trackers/clover/llvm/util.hpp index 8db6f20..fd47d3a
>> 100644
>> --- a/src/gallium/state_trackers/clover/llvm/util.hpp
>> +++ b/src/gallium/state_trackers/clover/llvm/util.hpp
>> @@ -40,12 +40,12 @@ namespace clover {
>>        }
>>
>>        inline std::vector<std::string>
>> -      tokenize(const std::string &s) {
>> +      tokenize(const std::string &s, const char sep = ' ') {
>>           std::vector<std::string> ss;
>>           std::istringstream iss(s);
>>           std::string t;
>>
>> -         while (getline(iss, t, ' '))
>> +         while (getline(iss, t, sep))
>>              ss.push_back(t);
>>
>>           return ss;
> 

-- 
Vedran Miletić
vedran.miletic.net
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to