Re: [Mesa-dev] [PATCH v2] clover: Allow OpenCL version override

[Francisco Jerez]

Vedran Miletić  writes:

> 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.
>
> v2:
>   - move version getters to version.hpp, simplify
>   - set -cl-std= to CLOVER_CL_C_VERSION_OVERRIDE
>   - set __OPENCL_VERSION__ to CLOVER_CL_VERSION_OVERRIDE
> ---
>  docs/envvars.html  | 12 ++
>  src/gallium/state_trackers/clover/Makefile.sources |  3 +-
>  src/gallium/state_trackers/clover/api/device.cpp   |  5 ++-
>  src/gallium/state_trackers/clover/api/platform.cpp |  3 +-
>  .../state_trackers/clover/llvm/invocation.cpp  | 30 --
>  src/gallium/state_trackers/clover/llvm/util.hpp|  4 +-
>  src/gallium/state_trackers/clover/util/version.hpp | 48 
> ++
>  7 files changed, 95 insertions(+), 10 deletions(-)
>  create mode 100644 src/gallium/state_trackers/clover/util/version.hpp
>
> 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.
>  
>  
> +Clover state tracker environment variables
> +
> +
> +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.

I'd change this to something along the lines of "allows overriding the
OpenCL API version reported by the OpenCL platform and devices, as
reported by clGetPlatformInfo(CL_PLATFORM_VERSION) and
clGetDeviceInfo(CL_DEVICE_VERSION)".

> +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 this is accurate, AFAIK __OPENCL_VERSION__ is supposed to
expand to the OpenCL API version (i.e. the value of
CLOVER_CL_VERSION_OVERRIDE), not the OpenCL C source language version.
Maybe use something along the lines of "allows overriding the source
language version version used by the OpenCL C compiler, as reported by
clGetDeviceInfo(CL_DEVICE_OPENCL_C_VERSION)".

You also probably need to rebase this on top of your previous commit.

> +
> +
>  Softpipe driver environment variables
>  
>  SOFTPIPE_DUMP_FS - if set, the softpipe driver will print fragment 
> shaders
> diff --git a/src/gallium/state_trackers/clover/Makefile.sources 
> b/src/gallium/state_trackers/clover/Makefile.sources
> index e9828b1..37f61b0 100644
> --- a/src/gallium/state_trackers/clover/Makefile.sources
> +++ b/src/gallium/state_trackers/clover/Makefile.sources
> @@ -50,7 +50,8 @@ CPP_SOURCES := \
>   util/lazy.hpp \
>   util/pointer.hpp \
>   util/range.hpp \
> - util/tuple.hpp
> + util/tuple.hpp \
> + util/version.hpp
>
I suggest you put this in api/util.hpp if you want to make them
stand-alone functions, since the clover/util library is largely
API-agnostic, and there's already some other CL version-related stuff in
api/util.hpp.  Though I have the impression that your first approach of
adding clover::platform and ::device queries is somewhat more
future-looking, since it will enable us to make the version computation
device-dependent instead of it being basically a constant.

>  LLVM_SOURCES := \
>   llvm/codegen/bitcode.cpp \
> diff --git a/src/gallium/state_trackers/clover/api/device.cpp 
> b/src/gallium/state_trackers/clover/api/device.cpp
> index f7bd61b..a759e0e 100644
> --- a/src/gallium/state_trackers/clover/api/device.cpp
> +++ b/src/gallium/state_trackers/clover/api/device.cpp
> @@ -23,6 +23,7 @@
>  #include "api/util.hpp"
>  #include "core/platform.hpp"
>  #include "core/device.hpp"
> +#include "util/version.hpp"
>  #include "git_sha1.h"
>  
>  using namespace clover;
> @@ -301,7 +302,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 " + get_opencl_version() + " Mesa " 
> PACKAGE_VERSION
>  #ifdef MESA_GIT_SHA1
>  " (" MESA_GIT_SHA1 ")"
>  #endif
> @@ -355,7 +356,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 " + get_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..f3360fe 100644
> --- a/src/gallium/state_trackers/clover/api/pla

Re: [Mesa-dev] [PATCH v2] clover: Allow OpenCL version override

[Jan Vesely]

On Fri, 2016-10-14 at 02:34 +0200, 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.
> 
> v2:
>   - move version getters to version.hpp, simplify
>   - set -cl-std= to CLOVER_CL_C_VERSION_OVERRIDE
>   - set __OPENCL_VERSION__ to CLOVER_CL_VERSION_OVERRIDE

other than a small nit in docs, LGTM.
Reviewed-by: Jan Vesely 
you probably want Francsisco's RB before pushing it.

> ---
>  docs/envvars.html  | 12 ++
>  src/gallium/state_trackers/clover/Makefile.sources |  3 +-
>  src/gallium/state_trackers/clover/api/device.cpp   |  5 ++-
>  src/gallium/state_trackers/clover/api/platform.cpp |  3 +-
>  .../state_trackers/clover/llvm/invocation.cpp  | 30 --
>  src/gallium/state_trackers/clover/llvm/util.hpp|  4 +-
>  src/gallium/state_trackers/clover/util/version.hpp | 48 
> ++
>  7 files changed, 95 insertions(+), 10 deletions(-)
>  create mode 100644 src/gallium/state_trackers/clover/util/version.hpp
> 
> 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.
>  
>  
> +Clover state tracker environment variables
> +
> +
> +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.
> +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.

Looking at the code, the macro is set by the former env var. clang will
set OPENCL_C_VERSION based on the lang option

Jan

> +
> +
>  Softpipe driver environment variables
>  
>  SOFTPIPE_DUMP_FS - if set, the softpipe driver will print fragment 
> shaders
> diff --git a/src/gallium/state_trackers/clover/Makefile.sources 
> b/src/gallium/state_trackers/clover/Makefile.sources
> index e9828b1..37f61b0 100644
> --- a/src/gallium/state_trackers/clover/Makefile.sources
> +++ b/src/gallium/state_trackers/clover/Makefile.sources
> @@ -50,7 +50,8 @@ CPP_SOURCES := \
>   util/lazy.hpp \
>   util/pointer.hpp \
>   util/range.hpp \
> - util/tuple.hpp
> + util/tuple.hpp \
> + util/version.hpp
>  
>  LLVM_SOURCES := \
>   llvm/codegen/bitcode.cpp \
> diff --git a/src/gallium/state_trackers/clover/api/device.cpp 
> b/src/gallium/state_trackers/clover/api/device.cpp
> index f7bd61b..a759e0e 100644
> --- a/src/gallium/state_trackers/clover/api/device.cpp
> +++ b/src/gallium/state_trackers/clover/api/device.cpp
> @@ -23,6 +23,7 @@
>  #include "api/util.hpp"
>  #include "core/platform.hpp"
>  #include "core/device.hpp"
> +#include "util/version.hpp"
>  #include "git_sha1.h"
>  
>  using namespace clover;
> @@ -301,7 +302,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 " + get_opencl_version() + " Mesa " 
> PACKAGE_VERSION
>  #ifdef MESA_GIT_SHA1
>  " (" MESA_GIT_SHA1 ")"
>  #endif
> @@ -355,7 +356,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 " + get_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..f3360fe 100644
> --- a/src/gallium/state_trackers/clover/api/platform.cpp
> +++ b/src/gallium/state_trackers/clover/api/platform.cpp
> @@ -22,6 +22,7 @@
>  
>  #include "api/util.hpp"
>  #include "core/platform.hpp"
> +#include "util/version.hpp"
>  #include "git_sha1.h"
>  
>  using namespace clover;
> @@ -58,7 +59,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 " + get_opencl_version() + " Mesa " 
> PACKAGE_VERSION
>  #ifdef MESA_GIT_SHA1
>  " (" MESA_GIT_SHA1 ")"
>  #endif
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp 
> b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index b5e8b52..6886dad 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_t