On Sunday 30 October 2016 16:07:25 Francisco Jerez wrote:
> Serge Martin <edb+m...@sigluy.net> writes:
> > ---
> >
> > src/gallium/state_trackers/clover/api/kernel.cpp | 47
> > ++++++++++++++++--
> > src/gallium/state_trackers/clover/core/kernel.cpp | 6 +++
> > src/gallium/state_trackers/clover/core/kernel.hpp | 1 +
> > src/gallium/state_trackers/clover/core/module.hpp | 19 +++++--
> > .../state_trackers/clover/llvm/codegen/common.cpp | 58
> > +++++++++++++++++++++- .../state_trackers/clover/llvm/metadata.hpp
> > | 16 ++++++
> > .../state_trackers/clover/tgsi/compiler.cpp | 2 +-
> > 7 files changed, 141 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/gallium/state_trackers/clover/api/kernel.cpp
> > b/src/gallium/state_trackers/clover/api/kernel.cpp index 73ba34a..13cfc08
> > 100644
> > --- a/src/gallium/state_trackers/clover/api/kernel.cpp
> > +++ b/src/gallium/state_trackers/clover/api/kernel.cpp
> > @@ -192,9 +192,50 @@ clGetKernelWorkGroupInfo(cl_kernel d_kern,
> > cl_device_id d_dev,>
> > CLOVER_API cl_int
> > clGetKernelArgInfo(cl_kernel d_kern,
> >
> > cl_uint idx, cl_kernel_arg_info param,
> >
> > - size_t size, void *r_buf, size_t *r_size) {
> > - CLOVER_NOT_SUPPORTED_UNTIL("1.2");
> > - return CL_KERNEL_ARG_INFO_NOT_AVAILABLE;
> > + size_t size, void *r_buf, size_t *r_size) try {
> > + property_buffer buf { r_buf, size, r_size };
> > + const auto &kern = obj(d_kern);
> > + const auto args_info = kern.args_info();
> > +
> > + if (args_info.size() == 0)
> > + throw error(CL_KERNEL_ARG_INFO_NOT_AVAILABLE);
> > +
> > + if (idx >= args_info.size())
> > + throw error(CL_INVALID_ARG_INDEX);
> > +
>
> I suggest you just handle std::out_of_range like clSetKernelArg does.
>
> > + const auto &info = args_info[idx];
> > +
> > + switch (param) {
> > + case CL_KERNEL_ARG_ADDRESS_QUALIFIER:
> > + buf.as_scalar<cl_kernel_arg_address_qualifier>() =
> > +
> > info.address_qualifier; + break;
> > +
> > + case CL_KERNEL_ARG_ACCESS_QUALIFIER:
> > + buf.as_scalar<cl_kernel_arg_access_qualifier>() =
> > +
> > info.access_qualifier; + break;
> > +
> > + case CL_KERNEL_ARG_TYPE_NAME:
> > + buf.as_string() = info.type_name;
> > + break;
> > +
> > + case CL_KERNEL_ARG_TYPE_QUALIFIER:
> > + buf.as_scalar<cl_kernel_arg_type_qualifier>() =
> > info.type_qualifier;
> > + break;
> > +
> > + case CL_KERNEL_ARG_NAME:
> > + buf.as_string() = info.arg_name;
> > + break;
> > +
> > + default:
> > + throw error(CL_INVALID_VALUE);
> > + }
> > +
> > + return CL_SUCCESS;
> > +
> > +} catch (error &e) {
> > + return e.get();
> >
> > }
> >
> > namespace {
> >
> > diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp
> > b/src/gallium/state_trackers/clover/core/kernel.cpp index
> > 962f555..18dcd5c 100644
> > --- a/src/gallium/state_trackers/clover/core/kernel.cpp
> > +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
> > @@ -140,6 +140,12 @@ kernel::args() const {
> >
> > return map(derefs(), _args);
> >
> > }
> >
> > +std::vector<clover::module::argument_info>
> > +kernel::args_info() const {
> > + const auto &syms = program().symbols();
> > + return find(name_equals(_name), syms).args_info;
> > +}
> > +
>
> This method saves literally one line of code in clGetKernelArgInfo.
> Maybe just open-code it?
>
> > const module &
> > kernel::module(const command_queue &q) const {
> >
> > return program().build(q.device()).binary;
> >
> > diff --git a/src/gallium/state_trackers/clover/core/kernel.hpp
> > b/src/gallium/state_trackers/clover/core/kernel.hpp index
> > 4ba6ff4..aae51bc 100644
> > --- a/src/gallium/state_trackers/clover/core/kernel.hpp
> > +++ b/src/gallium/state_trackers/clover/core/kernel.hpp
> > @@ -134,6 +134,7 @@ namespace clover {
> >
> > argument_range args();
> > const_argument_range args() const;
> >
> > + std::vector<clover::module::argument_info> args_info() const;
> >
> > const intrusive_ref<clover::program> program;
> >
> > diff --git a/src/gallium/state_trackers/clover/core/module.hpp
> > b/src/gallium/state_trackers/clover/core/module.hpp index
> > 5db0548..5ce9492 100644
> > --- a/src/gallium/state_trackers/clover/core/module.hpp
> > +++ b/src/gallium/state_trackers/clover/core/module.hpp
> > @@ -102,16 +102,29 @@ namespace clover {
> >
> > semantic semantic;
> >
> > };
> >
> > + struct argument_info {
> > + argument_info() { }
> > +
>
> If you define a constructor explicitly you should also explicitly
> initialize all POD members, otherwise their default value will be
> undefined...
>
> > + uint32_t address_qualifier;
>
> As Vedran pointed out, the computation of this value seems
> AMDGPU-specific, but in fact the field seems redundant because
> module::argument::type already gives you the same information:
>
> module::argument::local -> CL_KERNEL_ARG_ADDRESS_LOCAL
> module::argument::constant -> CL_KERNEL_ARG_ADDRESS_CONSTANT
> module::argument::global -> CL_KERNEL_ARG_ADDRESS_GLOBAL
> module::argument::image* -> CL_KERNEL_ARG_ADDRESS_GLOBAL
> otherwise -> CL_KERNEL_ARG_ADDRESS_PRIVATE
>
> > + uint32_t access_qualifier;
>
> This is also redundant with module::argument::type:
>
> module::argument::image*_rd -> CL_KERNEL_ARG_ACCESS_READ_ONLY
> module::argument::image*_wr -> CL_KERNEL_ARG_ACCESS_WRITE_ONLY
> otherwise -> CL_KERNEL_ARG_ACCESS_NONE
>
> > + std::string type_name;
> > + uint32_t type_qualifier;
>
> The module binary format is deliberately independent from CL API and
> LLVM-specific enumerants. I suggest you just pass the qualifiers
> through as a string like the other metadata fields of this struct.
>
> > + std::string arg_name;
> > + };
> > +
>
> I'd be inclined to nest this into a "struct info" member of "struct
> argument", which would enforce a one-to-one correspondence between info
> and argument structs. You could represent the info not being available
> as an empty (i.e. default-constructed) argument::info struct.
>
> > struct symbol {
> >
> > symbol(const std::string &name, resource_id section,
> >
> > - size_t offset, const std::vector<argument> &args) :
> > - name(name), section(section), offset(offset), args(args)
> > { } - symbol() : name(), section(0), offset(0), args() { }
> > + size_t offset, const std::vector<argument> &args,
> > + const std::vector<argument_info> &args_info) :
> > + name(name), section(section), offset(offset),
> > + args(args), args_info(args_info) { }
> > + symbol() : name(), section(0), offset(0), args(), args_info() {
> > }
> >
> > std::string name;
> > resource_id section;
> > size_t offset;
> > std::vector<argument> args;
> >
> > + std::vector<argument_info> args_info;
> >
> > };
> >
> > void serialize(std::ostream &os) const;
>
> Looks like you're missing some serialization code for the new info
> fields you've added to the module data structure.
This is on purpose. According to the spec : "Kernel argument information is
only available if the program object associated with kernel is created with
clCreateProgramWithSource "
Serge
>
> > diff --git a/src/gallium/state_trackers/clover/llvm/codegen/common.cpp
> > b/src/gallium/state_trackers/clover/llvm/codegen/common.cpp index
> > 834b06a..07f45f9 100644
> > --- a/src/gallium/state_trackers/clover/llvm/codegen/common.cpp
> > +++ b/src/gallium/state_trackers/clover/llvm/codegen/common.cpp
> > @@ -66,6 +66,60 @@ namespace {
> >
> > unreachable("Unknown image type");
> >
> > }
> >
> > + std::vector<module::argument_info>
> > + make_args_infos(const Module &mod, const std::string &kernel_name,
> > + const bool has_args_infos) {
>
> If you take my suggestion above to make module::argument_info part of
> module::argument you could just handle this from make_kernel_args (the
> has_args_infos boolean argument wouldn't be necessary since you could
> just ask the clang::CompilerInstance directly).
>
> > + if (!has_args_infos)
> > + return {};
> > +
> > + const Function &f = *mod.getFunction(kernel_name);
> > +
> > + std::vector<module::argument_info> infos;
> > + for (const auto &arg : f.args()) {
> > + module::argument_info info;
> > +
> > + const auto addr_space = get_uint_argument_metadata(f, arg,
> > +
> > "kernel_arg_addr_space"); + if (addr_space == 0)
> > + info.address_qualifier = CL_KERNEL_ARG_ADDRESS_PRIVATE;
> > + else if (addr_space == 1)
> > + info.address_qualifier = CL_KERNEL_ARG_ADDRESS_GLOBAL;
> > + else if (addr_space == 2)
> > + info.address_qualifier = CL_KERNEL_ARG_ADDRESS_CONSTANT;
> > + else if (addr_space == 3)
> > + info.address_qualifier = CL_KERNEL_ARG_ADDRESS_LOCAL;
> > +
> > + const auto access_qual = get_argument_metadata(f, arg,
> > +
> > "kernel_arg_access_qual"); + if (access_qual == "read_only")
> > + info.access_qualifier = CL_KERNEL_ARG_ACCESS_READ_ONLY;
> > + else if (access_qual == "write_only")
> > + info.access_qualifier = CL_KERNEL_ARG_ACCESS_WRITE_ONLY;
> > + else if (access_qual == "read_write")
> > + info.access_qualifier = CL_KERNEL_ARG_ACCESS_READ_WRITE;
> > + else if (access_qual == "none")
> > + info.access_qualifier = CL_KERNEL_ARG_ACCESS_NONE;
> > +
>
> The hunk above up to the beginning of the for loop wouldn't be necessary
> if you take my suggestion not to include redundant address and access
> qualifier fields in the argument info structure.
>
> > + info.type_name = get_argument_metadata(f, arg,
> > "kernel_arg_type"); +
> > + const auto type_qual = get_argument_metadata(f, arg,
> > +
> > "kernel_arg_type_qual"); + info.type_qualifier =
> > CL_KERNEL_ARG_TYPE_NONE;
> > + if (type_qual.find("const") != std::string::npos)
> > + info.type_qualifier |= CL_KERNEL_ARG_TYPE_CONST;
> > + if (type_qual.find("restrict") != std::string::npos)
> > + info.type_qualifier |= CL_KERNEL_ARG_TYPE_RESTRICT;
> > + if (type_qual.find("volatile") != std::string::npos)
> > + info.type_qualifier |= CL_KERNEL_ARG_TYPE_VOLATILE;
> > +
>
> This wouldn't be necessary if you pass the qualifiers through as a string.
>
> > + info.arg_name = get_argument_metadata(f, arg,
> > "kernel_arg_name");
> > +
> > + infos.emplace_back(info);
> > + }
> > +
> > + return infos;
> > + }
> > +
> >
> > std::vector<module::argument>
> > make_kernel_args(const Module &mod, const std::string &kernel_name,
> >
> > const clang::CompilerInstance &c) {
> >
> > @@ -196,12 +250,14 @@ clover::llvm::build_module_common(const Module &mod,
> >
> > unsigned> &offsets,
> >
> > const clang::CompilerInstance &c) {
> >
> > module m;
> >
> > + const bool has_args_infos = c.getCodeGenOpts().EmitOpenCLArgMetadata;
> >
> > for (const auto &name : map(std::mem_fn(&Function::getName),
> >
> > get_kernels(mod))) {
> >
> > if (offsets.count(name))
> >
> > m.syms.emplace_back(name, 0, offsets.at(name),
> >
> > - make_kernel_args(mod, name, c));
> > + make_kernel_args(mod, name, c),
> > + make_args_infos(mod, name, has_args_infos));
> >
> > }
> >
> > m.secs.push_back(make_text_section(code));
> >
> > diff --git a/src/gallium/state_trackers/clover/llvm/metadata.hpp
> > b/src/gallium/state_trackers/clover/llvm/metadata.hpp index
> > 814c830..ddc5259 100644
> > --- a/src/gallium/state_trackers/clover/llvm/metadata.hpp
> > +++ b/src/gallium/state_trackers/clover/llvm/metadata.hpp
> > @@ -32,6 +32,7 @@
> >
> > #include "util/algorithm.hpp"
> >
> > #include <vector>
> >
> > +#include <llvm/IR/Constants.h>
> >
> > #include <llvm/IR/Module.h>
> > #include <llvm/IR/Metadata.h>
> >
> > @@ -112,6 +113,21 @@ namespace clover {
> >
> > }
> >
> > ///
> >
> > + /// Extract the uint metadata node \p name corresponding to the
> > kernel + /// argument given by \p arg.
> > + ///
> > + inline uint64_t
> > + get_uint_argument_metadata(const ::llvm::Function &f,
> > + const ::llvm::Argument &arg,
> > + const std::string &name) {
> > + return ::llvm::cast<::llvm::ConstantInt>(
> > + ::llvm::dyn_cast<::llvm::ConstantAsMetadata>(
> > + detail::get_kernel_metadata_operands(f,
> > name)[arg.getArgNo()]) + ->getValue())
> > + ->getLimitedValue(UINT_MAX);
> > + }
> > +
>
> This helper function wouldn't be necessary if you take my other
> suggestions above.
>
> > + ///
> >
> > /// Return a vector with all CL kernel functions found in the LLVM
> > /// module \p mod.
> > ///
> >
> > diff --git a/src/gallium/state_trackers/clover/tgsi/compiler.cpp
> > b/src/gallium/state_trackers/clover/tgsi/compiler.cpp index
> > 9bbd454..f94d19f 100644
> > --- a/src/gallium/state_trackers/clover/tgsi/compiler.cpp
> > +++ b/src/gallium/state_trackers/clover/tgsi/compiler.cpp
> > @@ -76,7 +76,7 @@ namespace {
> >
> > }
> >
> > }
> >
> > - m.syms.push_back({ name, 0, offset, args });
> > + m.syms.push_back({ name, 0, offset, args, {} });
> >
> > }
> >
> > }
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
