[Mesa-dev] [PATCH v2] clover: add GetKernelArgInfo (CL 1.2)
Francisco Jerez
currojerez at riseup.net
Mon Nov 14 18:01:26 UTC 2016
Serge Martin <edb+mesa at sigluy.net> writes:
> On Sunday 30 October 2016 16:07:25 Francisco Jerez wrote:
>> Serge Martin <edb+mesa at 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 "
>
You're probably right, but clover::module is intended to be a direct
in-memory representation of a binary object format, with a 1:1
correspondence between the runtime and binary form, IOW I'd expect it to
respect the invariant:
| m.serialize(s);
| m == clover::module::deserialize(s);
If this bit of data couldn't be part of the binary representation of the
program I'd argue that it wouldn't belong here. Though I see how it's
convenient for you to make it part of the same data structure, because
it likely makes passing the extra argument information around easier.
I'd rather do one of the following alternatives in order to preserve the
expected behavior of clover::module:
- Store the argument metadata in clover::module with proper
serialization support, but throw CL_KERNEL_ARG_INFO_NOT_AVAILABLE
from clGetKernelArgInfo if the program source code isn't available
(you can check this by looking at the program::has_source boolean).
- Same as above, but allow querying argument information regardless of
whether the program was created from a binary or from source code as
an API extension.
> 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 at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161114/8fe9c33d/attachment-0001.sig>
More information about the mesa-dev
mailing list