[Mesa-dev] [PATCH 01/10] clover: Factor kernel argument parsing into its own function

Francisco Jerez currojerez at riseup.net
Tue Oct 7 02:31:18 PDT 2014


Tom Stellard <thomas.stellard at 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 <currojerez at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141007/2576851b/attachment.sig>


More information about the mesa-dev mailing list