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
