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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.


================
Comment at: clang/include/clang/Basic/Cuda.h:109
+  // Generic processor model is for testing only.
+  return A >= CudaArch::GFX600 && A <= CudaArch::GFX1035;
 }
----------------
can we use A < CudaArch::Generic instead? to avoid updating this line each time we add a new gfx arch.


================
Comment at: clang/include/clang/Driver/Options.td:1136
+def offload_EQ : CommaJoined<["--"], "offload=">, Flags<[NoXarchOption]>,
+  HelpText<"Specify comma-separated list of offloading targets.">;
+
----------------
linjamaki wrote:
> tra wrote:
> > `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.
> > 
> I’m not sure how to rephrase the option description to be more clear. In the [1] the `--offload` option is envisioned to be quite flexible/expressive - it can take in target triples, offload kinds, processors, aliases for processor sets, etc.
> 
> FYI, I have imagined that the `--offload` option would take in explicit offload kind and target triple combinations as the basis. For example, something like this:
> 
> 
> ```
> --offload=hip-amdgcn-amd-amdhsa,openmp-x86_64-pc-linux-gnu
> ```
> 
> And top of that, there would be predefined strings/shortcuts/aliases that expand to the basic form. For example, 
> `--offload=sm_70,openmp-host` could expand to something like:
> 
> 
> ```
> --offload=cuda-nvptx64-nvidia-cuda,openmp-x86_64-pc-linux-gnu -Xoffload=cuda-nvptx64-nvidia-cuda -march=sm_70 ...
> 
> ```
> Then there is a feature as discussed in [1] that the offload kind can be dropped if it can be inferred by other means (e.g. from `-x hip` option). 
> 
The description better matches the current implementation.

By this patch, `--offload=` only supports specifying target triple for HIP and assumes default processor. The description should reflect that.

In the future, as `--offload=` supports more values, the description may be updated.

Also, `--offload=` is designed to be mutually exclusive with `--offload-arch=`. Probably we should check and diagnose that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110622