Tom Stellard <thomas.stell...@amd.com> writes:
> ---
> .../state_trackers/clover/llvm/invocation.cpp | 142
> +++++++++++----------
> 1 file changed, 75 insertions(+), 67 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index 7bca0d6..3137591 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -293,96 +293,104 @@ namespace {
> PM.run(*mod);
> }
>
> - module
> - build_module_llvm(llvm::Module *mod,
> - const std::vector<llvm::Function *> &kernels,
> - clang::LangAS::Map& address_spaces) {
> -
> - module m;
> - struct pipe_llvm_program_header header;
> -
> - llvm::SmallVector<char, 1024> llvm_bitcode;
> - llvm::raw_svector_ostream bitcode_ostream(llvm_bitcode);
> - llvm::BitstreamWriter writer(llvm_bitcode);
> - llvm::WriteBitcodeToFile(mod, bitcode_ostream);
> - bitcode_ostream.flush();
> -
> - for (unsigned i = 0; i < kernels.size(); ++i) {
> - llvm::Function *kernel_func;
> - std::string kernel_name;
> - compat::vector<module::argument> args;
> + void
> + get_kernel_args(llvm::Module *mod, const std::string &kernel_name,
> + clang::LangAS::Map& address_spaces,
> + compat::vector<module::argument> &args) {
>
Style nit: As "args" is logically the result of this function, wouldn't
it be clearer to make it the return value? Like:
| compat::vector<module::argument>
| get_kernel_args(llvm::Module *mod, const std::string &kernel_name,
| clang::LangAS::Map& address_spaces);
(This is implemented by passing a pointer to the return object as an
implicit parameter anyway so it should be about as efficient)
> - kernel_func = kernels[i];
> - kernel_name = kernel_func->getName();
> + llvm::Function *kernel_func = mod->getFunction(kernel_name);
>
> - for (llvm::Function::arg_iterator I = kernel_func->arg_begin(),
> + for (llvm::Function::arg_iterator I = kernel_func->arg_begin(),
> E = kernel_func->arg_end(); I != E;
> ++I) {
> - llvm::Argument &arg = *I;
> + llvm::Argument &arg = *I;
> #if HAVE_LLVM < 0x0302
> - llvm::TargetData TD(kernel_func->getParent());
> + llvm::TargetData TD(kernel_func->getParent());
> #elif HAVE_LLVM < 0x0305
> - llvm::DataLayout TD(kernel_func->getParent()->getDataLayout());
> + llvm::DataLayout TD(kernel_func->getParent()->getDataLayout());
> #else
> - llvm::DataLayout TD(mod);
> + llvm::DataLayout TD(mod);
> #endif
>
> - llvm::Type *arg_type = arg.getType();
> - const unsigned arg_store_size = TD.getTypeStoreSize(arg_type);
> + llvm::Type *arg_type = arg.getType();
> + const unsigned arg_store_size = TD.getTypeStoreSize(arg_type);
>
> - // OpenCL 1.2 specification, Ch. 6.1.5: "A built-in data
> - // type that is not a power of two bytes in size must be
> - // aligned to the next larger power of two". We need this
> - // alignment for three element vectors, which have
> - // non-power-of-2 store size.
> - const unsigned arg_api_size =
> - util_next_power_of_two(arg_store_size);
> + // OpenCL 1.2 specification, Ch. 6.1.5: "A built-in data
> + // type that is not a power of two bytes in size must be
> + // aligned to the next larger power of two". We need this
> + // alignment for three element vectors, which have
> + // non-power-of-2 store size.
> + const unsigned arg_api_size =
> util_next_power_of_two(arg_store_size);
>
> - llvm::Type *target_type = arg_type->isIntegerTy() ?
> + llvm::Type *target_type = arg_type->isIntegerTy() ?
> TD.getSmallestLegalIntType(mod->getContext(), arg_store_size
> * 8)
> : arg_type;
> - unsigned target_size = TD.getTypeStoreSize(target_type);
> - unsigned target_align = TD.getABITypeAlignment(target_type);
> + unsigned target_size = TD.getTypeStoreSize(target_type);
> + unsigned target_align = TD.getABITypeAlignment(target_type);
>
> - if (llvm::isa<llvm::PointerType>(arg_type) &&
> arg.hasByValAttr()) {
> - arg_type =
> + if (llvm::isa<llvm::PointerType>(arg_type) && arg.hasByValAttr()) {
> + arg_type =
>
> llvm::dyn_cast<llvm::PointerType>(arg_type)->getElementType();
> - }
> + }
>
> - if (arg_type->isPointerTy()) {
> - unsigned address_space =
> llvm::cast<llvm::PointerType>(arg_type)->getAddressSpace();
> - if (address_space ==
> address_spaces[clang::LangAS::opencl_local
> + if (arg_type->isPointerTy()) {
> + unsigned address_space =
> llvm::cast<llvm::PointerType>(arg_type)->getAddressSpace();
> + if (address_space == address_spaces[clang::LangAS::opencl_local
> -
> clang::LangAS::Offset]) {
> - args.push_back(module::argument(module::argument::local,
> - arg_api_size, target_size,
> - target_align,
> -
> module::argument::zero_ext));
> - } else {
> - // XXX: Correctly handle constant address space. There is
> no
> - // way for r600g to pass a handle for constant buffers back
> - // to clover like it can for global buffers, so
> - // creating constant arguments will break r600g. For now,
> - // continue treating constant buffers as global buffers
> - // until we can come up with a way to create handles for
> - // constant buffers.
> - args.push_back(module::argument(module::argument::global,
> - arg_api_size, target_size,
> - target_align,
> -
> module::argument::zero_ext));
> - }
> -
> + args.push_back(module::argument(module::argument::local,
> + arg_api_size, target_size,
> + target_align,
> + module::argument::zero_ext));
> } else {
> - llvm::AttributeSet attrs = kernel_func->getAttributes();
> - enum module::argument::ext_type ext_type =
> + // XXX: Correctly handle constant address space. There is no
> + // way for r600g to pass a handle for constant buffers back
> + // to clover like it can for global buffers, so
> + // creating constant arguments will break r600g. For now,
> + // continue treating constant buffers as global buffers
> + // until we can come up with a way to create handles for
> + // constant buffers.
> + args.push_back(module::argument(module::argument::global,
> + arg_api_size, target_size,
> + target_align,
> + module::argument::zero_ext));
> + }
> +
> + } else {
> + llvm::AttributeSet attrs = kernel_func->getAttributes();
> + enum module::argument::ext_type ext_type =
> (attrs.hasAttribute(arg.getArgNo() + 1,
> llvm::Attribute::SExt) ?
> module::argument::sign_ext :
> module::argument::zero_ext);
>
> - args.push_back(
> - module::argument(module::argument::scalar, arg_api_size,
> - target_size, target_align, ext_type));
> - }
> + args.push_back(
> + module::argument(module::argument::scalar, arg_api_size,
> + target_size, target_align, ext_type));
> }
> + }
> + }
> +
> + module
> + build_module_llvm(llvm::Module *mod,
> + const std::vector<llvm::Function *> &kernels,
> + clang::LangAS::Map& address_spaces) {
> +
> + module m;
> + struct pipe_llvm_program_header header;
> +
> + llvm::SmallVector<char, 1024> llvm_bitcode;
> + llvm::raw_svector_ostream bitcode_ostream(llvm_bitcode);
> + llvm::BitstreamWriter writer(llvm_bitcode);
> + llvm::WriteBitcodeToFile(mod, bitcode_ostream);
> + bitcode_ostream.flush();
> +
> + for (unsigned i = 0; i < kernels.size(); ++i) {
> + llvm::Function *kernel_func;
> + std::string kernel_name;
> + compat::vector<module::argument> args;
> +
> + kernel_func = kernels[i];
> + kernel_name = kernel_func->getName();
We probably don't need the kernel_func variable anymore if you coalesce
both assignments like:
| std:string kernel_name = kernels[i]->getName();
Other than that, this looks good,
Reviewed-by: Francisco Jerez <curroje...@riseup.net>
> + get_kernel_args(mod, kernel_name, address_spaces, args);
>
> m.syms.push_back(module::symbol(kernel_name, 0, i, args ));
> }
> --
> 1.8.5.5
pgpXwk2gAloz5.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
