[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

> --offload’ option, which is envisioned in [1], is added for specifying offload targets. This option is used to override default device target (amdgcn-amd-amdhsa) for HIP compilation for emitting device code as SPIR-V binary. The option is handled in getHIPOffloadTargetTriple().

Can you elaborate on what exactly this option does and how it's intended to interact with the existing `--offload-arch`?

In general a list of values, combined with the `getLastArg` will potentially be an issue if/when more than one list value will be supported.
In a large build it's fairly common for the build infrastructure to set the default options and allowing users to extend/override them with *additional* options. `getLastArg` works great for scalar options, not so much for the lists.
If an option is a list, modifying it requires prior knowledge of preceding values and that may not always be easy.
E.g. a build configuration may be set to  target gfx900 and gfx908. If I want to *add* an option to target gfx1030, I would need to dig out the options for the currently-enabled architectures and specify all of them again. It's doable once, manually, but it does not scale if this option is expected to be regularly tweaked by the end user, as is the case with `--offload-arch`. If `--offload` is expected to have similar use patterns, you may need to consider allowing it to be adjusted per-list-element.



================
Comment at: clang/include/clang/Basic/Cuda.h:106
 static inline bool IsAMDGpuArch(CudaArch A) {
   return A >= CudaArch::GFX600 && A < CudaArch::LAST;
 }
----------------
Does this need to be adjusted to exclude SPIR-V? If so, you may want to add another enum range for SPIR-V.



================
Comment at: clang/include/clang/Driver/Options.td:1136
+def offload_EQ : CommaJoined<["--"], "offload=">, Flags<[NoXarchOption]>,
+  HelpText<"Specify comma-separated list of offloading targets.">;
+
----------------
`comma-separated list of offloading targets.` is, unfortunately, somewhat ambiguous.
Does it mean "how the offload will be done". I.e. HSA, OpenMP, SPIRV, CUDA? 
Or does it mean specific hardware we need to generate the code for? 
The code suggests it's a variant of the former, but the option description does not. 

E.g. `offload_arch_EQ ` also uses the term "offloading target" but with a different meaning.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110622