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.
> 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, {} });
> }
> }
>
> --
> 2.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
