[Mesa-dev] [PATCH 5/5] clover: Enable cl_khr_fp64 for devices that support doubles v2
Tom Stellard
tom at stellard.net
Fri Jul 4 08:06:52 PDT 2014
On Fri, Jul 04, 2014 at 12:28:05PM +0200, Francisco Jerez wrote:
> Tom Stellard <tom at stellard.net> writes:
>
> > On Fri, Jul 04, 2014 at 12:28:20AM +0200, Francisco Jerez wrote:
> >> Tom Stellard <tom at stellard.net> writes:
> >>
> >> > On Thu, Jul 03, 2014 at 01:12:07AM +0200, Francisco Jerez wrote:
> >> >> Tom Stellard <tom at stellard.net> writes:
> >> >>
> >> >> > On Thu, Jun 26, 2014 at 04:15:39PM +0200, Francisco Jerez wrote:
> >> >> >> Tom Stellard <thomas.stellard at amd.com> writes:
> >> >> >>
> >> >> >> > v2:
> >> >> >> > - Report correct values for CL_DEVICE_NATIVE_VECTOR_WIDTH_DOUBLE and
> >> >> >> > CL_DEVICE_PREFERRED_VECTOR_WIDTH_DOUBLE.
> >> >> >> > - Only define cl_khr_fp64 if the extension is supported.
> >> >> >> > - Remove trailing space from extension string.
> >> >> >> > - Rename device query function from cl_khr_fp86() to has_doubles().
> >> >> >> > ---
> >> >> >> > src/gallium/state_trackers/clover/api/device.cpp | 6 +++---
> >> >> >> > src/gallium/state_trackers/clover/core/device.cpp | 6 ++++++
> >> >> >> > src/gallium/state_trackers/clover/core/device.hpp | 1 +
> >> >> >> > src/gallium/state_trackers/clover/core/program.cpp | 5 ++++-
> >> >> >> > src/gallium/state_trackers/clover/llvm/invocation.cpp | 1 -
> >> >> >> > 5 files changed, 14 insertions(+), 5 deletions(-)
> >> >> >> >
> >> >> >> > diff --git a/src/gallium/state_trackers/clover/api/device.cpp b/src/gallium/state_trackers/clover/api/device.cpp
> >> >> >> > index 7006702..1176668 100644
> >> >> >> > --- a/src/gallium/state_trackers/clover/api/device.cpp
> >> >> >> > +++ b/src/gallium/state_trackers/clover/api/device.cpp
> >> >> >> > @@ -145,7 +145,7 @@ clGetDeviceInfo(cl_device_id d_dev, cl_device_info param,
> >> >> >> > break;
> >> >> >> >
> >> >> >> > case CL_DEVICE_PREFERRED_VECTOR_WIDTH_DOUBLE:
> >> >> >> > - buf.as_scalar<cl_uint>() = 2;
> >> >> >> > + buf.as_scalar<cl_uint>() = dev.has_doubles() ? 2 : 0;
> >> >> >> > break;
> >> >> >> >
> >> >> >> > case CL_DEVICE_PREFERRED_VECTOR_WIDTH_HALF:
> >> >> >> > @@ -290,7 +290,7 @@ clGetDeviceInfo(cl_device_id d_dev, cl_device_info param,
> >> >> >> > break;
> >> >> >> >
> >> >> >> > case CL_DEVICE_EXTENSIONS:
> >> >> >> > - buf.as_string() = "";
> >> >> >> > + buf.as_string() = dev.has_doubles() ? "cl_khr_fp64" : "";
> >> >> >> > break;
> >> >> >> >
> >> >> >> > case CL_DEVICE_PLATFORM:
> >> >> >> > @@ -322,7 +322,7 @@ clGetDeviceInfo(cl_device_id d_dev, cl_device_info param,
> >> >> >> > break;
> >> >> >> >
> >> >> >> > case CL_DEVICE_NATIVE_VECTOR_WIDTH_DOUBLE:
> >> >> >> > - buf.as_scalar<cl_uint>() = 2;
> >> >> >> > + buf.as_scalar<cl_uint>() = dev.has_doubles() ? 2 : 0;
> >> >> >> > break;
> >> >> >> >
> >> >> >> > case CL_DEVICE_NATIVE_VECTOR_WIDTH_HALF:
> >> >> >> > diff --git a/src/gallium/state_trackers/clover/core/device.cpp b/src/gallium/state_trackers/clover/core/device.cpp
> >> >> >> > index bc6b761..6bf33e0 100644
> >> >> >> > --- a/src/gallium/state_trackers/clover/core/device.cpp
> >> >> >> > +++ b/src/gallium/state_trackers/clover/core/device.cpp
> >> >> >> > @@ -193,6 +193,12 @@ device::half_fp_config() const {
> >> >> >> > return CL_FP_DENORM | CL_FP_INF_NAN | CL_FP_ROUND_TO_NEAREST;
> >> >> >> > }
> >> >> >> >
> >> >> >> > +bool
> >> >> >> > +device::has_doubles() const {
> >> >> >> > + return pipe->get_shader_param(pipe, PIPE_SHADER_COMPUTE,
> >> >> >> > + PIPE_SHADER_CAP_DOUBLES);
> >> >> >> > +}
> >> >> >> > +
> >> >> >> > std::vector<size_t>
> >> >> >> > device::max_block_size() const {
> >> >> >> > auto v = get_compute_param<uint64_t>(pipe, PIPE_COMPUTE_CAP_MAX_BLOCK_SIZE);
> >> >> >> > diff --git a/src/gallium/state_trackers/clover/core/device.hpp b/src/gallium/state_trackers/clover/core/device.hpp
> >> >> >> > index 16831ab..025c648 100644
> >> >> >> > --- a/src/gallium/state_trackers/clover/core/device.hpp
> >> >> >> > +++ b/src/gallium/state_trackers/clover/core/device.hpp
> >> >> >> > @@ -66,6 +66,7 @@ namespace clover {
> >> >> >> > cl_device_fp_config single_fp_config() const;
> >> >> >> > cl_device_fp_config double_fp_config() const;
> >> >> >> > cl_device_fp_config half_fp_config() const;
> >> >> >> > + bool has_doubles() const;
> >> >> >> >
> >> >> >> > std::vector<size_t> max_block_size() const;
> >> >> >> > std::string device_name() const;
> >> >> >> > diff --git a/src/gallium/state_trackers/clover/core/program.cpp b/src/gallium/state_trackers/clover/core/program.cpp
> >> >> >> > index e09c3aa..f65f321 100644
> >> >> >> > --- a/src/gallium/state_trackers/clover/core/program.cpp
> >> >> >> > +++ b/src/gallium/state_trackers/clover/core/program.cpp
> >> >> >> > @@ -95,7 +95,10 @@ program::build_status(const device &dev) const {
> >> >> >> >
> >> >> >> > std::string
> >> >> >> > program::build_opts(const device &dev) const {
> >> >> >> > - return _opts.count(&dev) ? _opts.find(&dev)->second : "";
> >> >> >> > + std::string opts = _opts.count(&dev) ? _opts.find(&dev)->second : "";
> >> >> >> > + if (dev.has_doubles())
> >> >> >> > + opts.append(" -Dcl_khr_fp64");
> >> >> >> > + return opts;
> >> >> >>
> >> >> >> This define belongs in the target-specific part of libclc. With this
> >> >> >> hunk removed this patch is:
> >> >> >>
> >> >> >
> >> >> > The declarations for double functions in the libclc headers are wrapped in this
> >> >> > macro, so we need to set it here in order to be able to use them from clover.
> >> >> >
> >> >>
> >> >> This abuses the ::build_opts() accessor to that end, which is only
> >> >> supposed to return the compiler options that were specified by the user
> >> >> at build time, as required by the CL_PROGRAM_BUILD_OPTIONS build param.
> >> >>
> >> >
> >> > You are right, I can fix that.
> >> >
> >> >> IMO preprocessor macros defined by the spec belong in the standard
> >> >> library. We probably need a specialization of libclc's header files for
> >> >> each triple (I hadn't noticed you didn't have one already -- it will
> >> >> probably be useful for other reasons too), as you have target-specific
> >> >> specializations of the LLVM bitcode library.
> >> >>
> >> >
> >> > What sort of target specific things should go in the headers?
> >> > Currently, all target-specific code goes into the library.
> >> >
> >>
> >> Wouldn't such a thing be useful for other device-dependent numeric
> >> constants and defines, and for built-in functions that either expand to
> >> a generic in-line implementation or a more efficient device-specific
> >> implementation (possibly using compiler intrinsics)? Just like the C
> >> standard library.
> >>
> >
> > I don't understand why device-dependent constants would go in the libclc
> > header files. Wouldn't they be private to the library implementation?
>
> I mean device-dependent preprocessor constants. E.g. how are you
> planning to implement FP_FAST_FMA? It's device-dependent but it can't
> be implemented in the bitcode library because it's a preprocessor macro.
>
You can add these macros to your target definition in clang and they will
be defined automatically.
> > Also, libclc does use complier intrinsics, but these are all implemented
> > in the library and not in the header files. There is a
> > separate library built for each device.
> >
> Isn't it pretty annoying to write LLVM IR by hand? Wouldn't it be
> easier and more efficient to call the intrinsics from the header
> directly?
>
Most of the library is written in OpenCL C. You can access almost all
the intrinsics using the clang __builtin functions if you need to.
I'm still not sure how it would work to have device dependent function
calls in the headers. Wouldn't we still need to append something like
-D__BONAIRE_GPU to the compiler arguments when we invoke clang?
-Tom
> > -Tom
> >
> >
> >
> >> > -Tom
> >> >
> >> >
> >> >> Thanks.
> >> >>
> >> >> > -Tom
> >> >> >
> >> >> >> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> >> >> >>
> >> >> >> > }
> >> >> >> >
> >> >> >> > std::string
> >> >> >> > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >> >> >> > index 5d2efc4..f2b4fd9 100644
> >> >> >> > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >> >> >> > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >> >> >> > @@ -183,7 +183,6 @@ namespace {
> >> >> >> >
> >> >> >> > // clc.h requires that this macro be defined:
> >> >> >> > c.getPreprocessorOpts().addMacroDef("cl_clang_storage_class_specifiers");
> >> >> >> > - c.getPreprocessorOpts().addMacroDef("cl_khr_fp64");
> >> >> >> >
> >> >> >> > c.getLangOpts().NoBuiltin = true;
> >> >> >> > c.getTargetOpts().Triple = triple;
> >> >> >> > --
> >> >> >> > 1.8.1.5
> >> >> >> >
> >> >> >> > _______________________________________________
> >> >> >> > mesa-dev mailing list
> >> >> >> > mesa-dev at lists.freedesktop.org
> >> >> >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> >> _______________________________________________
> >> >> >> mesa-dev mailing list
> >> >> >> mesa-dev at lists.freedesktop.org
> >> >> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list