[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:34:40 PDT 2014
On Fri, Jul 04, 2014 at 05:25:42PM +0200, Francisco Jerez wrote:
> Tom Stellard <tom at stellard.net> writes:
>
> > 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.
> >
>
> Right, that would work too. Couldn't we do the same with extension
> macros like this one? (as they're more of a language feature than a
> property of the host library, so IMHO clover shouldn't have to care
> about them)
>
This is an acceptable solution for me, I will do it this way.
-Tom
> >
> >> > 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.
> >
> Right.
>
> > 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?
> >
>
> That would work too I guess, but I was thinking we could have headers
> for different architectures installed under different prefixes and have
> clover pick the right one based on the target triple.
>
> >
> > -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