[Mesa-dev] [PATCH 1/2] clover: Added missing address space checking of kernel parameters

Tom Stellard tom at stellard.net
Thu Jul 25 19:25:04 PDT 2013


Hi Francisco,

I just realized that this breaks constant buffers on r600g.  The main
problem is that there is no way for r600g to return a handle for
constant buffers like it can for global buffers.  This is also a
problem for sampler_views and resources, which are used for images.  Do
you have any suggestions for how to resolve this?  Would it work to just
add a handle argument to set_compute_sampler_views and
set_compute_resources?

-Tom


On Wed, Jul 24, 2013 at 09:29:49AM -0400, Jonathan Charest wrote:
> Here is an updated patch with no line wrapping and respecting 80-column limit (for my changes).
> 
> ---
>  .../state_trackers/clover/llvm/invocation.cpp      | 34 +++++++++++++++-------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index dae61f7..3846a6e 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -26,6 +26,7 @@
>  #include <clang/Frontend/TextDiagnosticBuffer.h>
>  #include <clang/Frontend/TextDiagnosticPrinter.h>
>  #include <clang/CodeGen/CodeGenAction.h>
> +#include <clang/Basic/TargetInfo.h>
>  #include <llvm/Bitcode/BitstreamWriter.h>
>  #include <llvm/Bitcode/ReaderWriter.h>
>  #include <llvm/Linker.h>
> @@ -113,7 +114,7 @@ namespace {
>     llvm::Module *
>     compile(const std::string &source, const std::string &name,
>             const std::string &triple, const std::string &processor,
> -           const std::string &opts) {
> +           const std::string &opts, clang::LangAS::Map& address_spaces) {
>  
>        clang::CompilerInstance c;
>        clang::CompilerInvocation invocation;
> @@ -204,6 +205,10 @@ namespace {
>        if (!c.ExecuteAction(act))
>           throw build_error(log);
>  
> +      // Get address spaces map to be able to find kernel argument address space
> +      memcpy(address_spaces, c.getTarget().getAddressSpaceMap(), 
> +                                                        sizeof(address_spaces));
> +
>        return act.takeModule();
>     }
>  
> @@ -282,7 +287,8 @@ namespace {
>  
>     module
>     build_module_llvm(llvm::Module *mod,
> -                     const std::vector<llvm::Function *> &kernels) {
> +                     const std::vector<llvm::Function *> &kernels,
> +                     clang::LangAS::Map& address_spaces) {
>  
>        module m;
>        struct pipe_llvm_program_header header;
> @@ -318,14 +324,18 @@ namespace {
>              }
>  
>              if (arg_type->isPointerTy()) {
> -               // XXX: Figure out LLVM->OpenCL address space mappings for each
> -               // target.  I think we need to ask clang what these are.  For now,
> -               // pretend everything is in the global address space.
>                 unsigned address_space = llvm::cast<llvm::PointerType>(arg_type)->getAddressSpace();
> -               switch (address_space) {
> -                  default:
> -                     args.push_back(module::argument(module::argument::global, arg_size));
> -                     break;
> +               if (address_space == address_spaces[clang::LangAS::opencl_local
> +                                                     - clang::LangAS::Offset]) {
> +                  args.push_back(module::argument(module::argument::local, 
> +                                                                     arg_size));
> +               } else if (address_space == address_spaces[
> +                     clang::LangAS::opencl_constant  - clang::LangAS::Offset]) {
> +                  args.push_back(module::argument(module::argument::constant, 
> +                                                                     arg_size));
> +               } else {
> +                  args.push_back(module::argument(module::argument::global, 
> +                                                                     arg_size));
>                 }
>              } else {
>                 args.push_back(module::argument(module::argument::scalar, arg_size));
> @@ -358,10 +368,12 @@ clover::compile_program_llvm(const compat::string &source,
>     std::string processor(target.begin(), 0, processor_str_len);
>     std::string triple(target.begin(), processor_str_len + 1,
>                        target.size() - processor_str_len - 1);
> +   clang::LangAS::Map address_spaces;
>  
>     // The input file name must have the .cl extension in order for the
>     // CompilerInvocation class to recognize it as an OpenCL source file.
> -   llvm::Module *mod = compile(source, "input.cl", triple, processor, opts);
> +   llvm::Module *mod = compile(source, "input.cl", triple, processor, opts,
> +                                                                address_spaces);
>  
>     find_kernels(mod, kernels);
>  
> @@ -374,6 +386,6 @@ clover::compile_program_llvm(const compat::string &source,
>           assert(0);
>           return module();
>        default:
> -         return build_module_llvm(mod, kernels);
> +         return build_module_llvm(mod, kernels, address_spaces);
>     }
>  }
> -- 
> 1.8.3.3
> 
> 
> On 2013-07-24 03:58, Francisco Jerez wrote:
> > Tom Stellard <tom at stellard.net> writes:
> > 
> >> On Mon, Jul 22, 2013 at 09:24:12AM -0400, Jonathan Charest wrote:
> >>> To have non-static buffers in local memory, it is necessary to pass them
> >>> as arguments to the kernel. This was almost supported but the address
> >>> space mapping was missing to perform the check (thanks to tstellar for
> >>> pointing me in the right direction).
> >>>
> >>> ---
> >>>  .../state_trackers/clover/llvm/invocation.cpp      | 31
> >>> ++++++++++++++--------
> >>
> >> Reviewed-by: Tom Stellard <thomas.stellard at amd.com>
> >>
> > Looks OK to me, but the patch doesn't apply because of the line
> > wrapping.  Also make sure you place block braces consistently and
> > respect the 80-column limit as far as possible.
> > 
> >>>  1 file changed, 20 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >>> b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >>> index dae61f7..5cb5724 100644
> >>> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >>> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> >>> @@ -26,6 +26,7 @@
> >>>  #include <clang/Frontend/TextDiagnosticBuffer.h>
> >>>  #include <clang/Frontend/TextDiagnosticPrinter.h>
> >>>  #include <clang/CodeGen/CodeGenAction.h>
> >>> +#include <clang/Basic/TargetInfo.h>
> >>>  #include <llvm/Bitcode/BitstreamWriter.h>
> >>>  #include <llvm/Bitcode/ReaderWriter.h>
> >>>  #include <llvm/Linker.h>
> >>> @@ -113,7 +114,7 @@ namespace {
> >>>     llvm::Module *
> >>>     compile(const std::string &source, const std::string &name,
> >>>             const std::string &triple, const std::string &processor,
> >>> -           const std::string &opts) {
> >>> +           const std::string &opts, clang::LangAS::Map& address_spaces) {
> >>>
> >>>        clang::CompilerInstance c;
> >>>        clang::CompilerInvocation invocation;
> >>> @@ -204,6 +205,9 @@ namespace {
> >>>        if (!c.ExecuteAction(act))
> >>>           throw build_error(log);
> >>>
> >>> +      // Get address spaces map to be able to find kernel argument
> >>> address spaces
> >>> +      memcpy(address_spaces, c.getTarget().getAddressSpaceMap(),
> >>> sizeof(address_spaces));
> >>> +
> >>>        return act.takeModule();
> >>>     }
> >>>
> >>> @@ -282,7 +286,8 @@ namespace {
> >>>
> >>>     module
> >>>     build_module_llvm(llvm::Module *mod,
> >>> -                     const std::vector<llvm::Function *> &kernels) {
> >>> +                     const std::vector<llvm::Function *> &kernels,
> >>> +                     clang::LangAS::Map& address_spaces) {
> >>>
> >>>        module m;
> >>>        struct pipe_llvm_program_header header;
> >>> @@ -318,14 +323,17 @@ namespace {
> >>>              }
> >>>
> >>>              if (arg_type->isPointerTy()) {
> >>> -               // XXX: Figure out LLVM->OpenCL address space mappings
> >>> for each
> >>> -               // target.  I think we need to ask clang what these are.
> >>>  For now,
> >>> -               // pretend everything is in the global address space.
> >>>                 unsigned address_space =
> >>> llvm::cast<llvm::PointerType>(arg_type)->getAddressSpace();
> >>> -               switch (address_space) {
> >>> -                  default:
> >>> -
> >>> args.push_back(module::argument(module::argument::global, arg_size));
> >>> -                     break;
> >>> +               if (address_space ==
> >>> +                   address_spaces[clang::LangAS::opencl_local -
> >>> clang::LangAS::Offset]) {
> >>> +
> >>> args.push_back(module::argument(module::argument::local, arg_size));
> >>> +               }
> >>> +               else if (address_space ==
> >>> +                        address_spaces[clang::LangAS::opencl_constant -
> >>> clang::LangAS::Offset]) {
> >>> +
> >>> args.push_back(module::argument(module::argument::constant, arg_size));
> >>> +               }
> >>> +               else {
> >>> +
> >>> args.push_back(module::argument(module::argument::global, arg_size));
> >>>                 }
> >>>              } else {
> >>>
> >>> args.push_back(module::argument(module::argument::scalar, arg_size));
> >>> @@ -358,10 +366,11 @@ clover::compile_program_llvm(const compat::string
> >>> &source,
> >>>     std::string processor(target.begin(), 0, processor_str_len);
> >>>     std::string triple(target.begin(), processor_str_len + 1,
> >>>                        target.size() - processor_str_len - 1);
> >>> +   clang::LangAS::Map address_spaces;
> >>>
> >>>     // The input file name must have the .cl extension in order for the
> >>>     // CompilerInvocation class to recognize it as an OpenCL source file.
> >>> -   llvm::Module *mod = compile(source, "input.cl", triple, processor,
> >>> opts);
> >>> +   llvm::Module *mod = compile(source, "input.cl", triple, processor,
> >>> opts, address_spaces);
> >>>
> >>>     find_kernels(mod, kernels);
> >>>
> >>> @@ -374,6 +383,6 @@ clover::compile_program_llvm(const compat::string
> >>> &source,
> >>>           assert(0);
> >>>           return module();
> >>>        default:
> >>> -         return build_module_llvm(mod, kernels);
> >>> +         return build_module_llvm(mod, kernels, address_spaces);
> >>>     }
> >>>  }
> >>> -- 1.8.3.2
> >>>
> >>> _______________________________________________
> >>> mesa-dev mailing list
> >>> mesa-dev at lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 


More information about the mesa-dev mailing list