[PATCH] D110618: [HIPSPV][2/4] Add HIPSPV tool chain

Henry Linjamäki via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 4 01:11:10 PDT 2021


linjamaki added a comment.

In D110618#3032899 <https://reviews.llvm.org/D110618#3032899>, @Anastasia wrote:

> Considering that SPIR-V translation step is also required for other languages would it make sense to add `llvm-spirv` as a common tool like for example C/C++ linkers and create a bit of common infrastructure? It might be something we can do as a separate step too but it would be good to make sure nothing prevents us from doing this in the future... We should probably also think about the command line interface unification as it probably doesn't make sense for every language to add their own flags for locating SPIR-V translator.

I don’t think it would work out well. llvm-spirv (or other tool relying on LLVM bitcode input) and LLVM may have version differences that cause obscure error cases - like newer LLVM producing bitcode with new features the tools are not expecting and ready for them. The usage of llvm-spirv tool in the HIPSPV tool chain is not intended to be a longer term solution due to this shortcoming.

> Another question I have is whether `--hipspv-pass-plugin` would be needed when we switch to SPIR-V backend or is this something that would be integrated fully upstream eventually. Then we might need to think more of the suitable transition path.

It’s too early to say if we can integrate --hipspv-pass-plugin fully in the future. It would be the preferred outcome. We are not finished with the HIP expanders we need for supporting various HIP features so we don’t know for certain what will be the outcome for the integration. It’s possible that some of the solutions are going to be tightly coupled with runtimes (HIPLZ, HIPCL) and others that may not generalize for different SPIR-V execution environments.



================
Comment at: clang/include/clang/Driver/Options.td:3701
   " do not include the default CUDA/HIP wrapper headers">;
+def nohipwrapperinc : Flag<["-"], "nohipwrapperinc">,
+  HelpText<"Do not include the default HIP wrapper headers">;
----------------
tra wrote:
> Is the idea to still add relevant include paths to the wrappers and SDK headers, but not `-include` the wrapper?
> 
> If that's the case, it should probably be generalized into `-nogpuwrapperinc` and apply to both CUDA and HIP.
> 
> Is the idea to still add relevant include paths to the wrappers and SDK headers, but not `-include` the wrapper?
> 
Include paths are meant to be excluded too. I’ll fix the option description.

> If that's the case, it should probably be generalized into `-nogpuwrapperinc` and apply to both CUDA and HIP.
> 
I don’t see an immediate need to generalize the option as I don’t think there will be a need for it in the CUDA path. The option could be generalized later if the need comes (add generalized option, set -nohipwrapperinc to be alias to it).



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110618/new/

https://reviews.llvm.org/D110618



More information about the cfe-commits mailing list