[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/lib/Driver/Driver.cpp:105-109
+    auto HIPOffloadTargets = Args.getAllArgValues(options::OPT_offload_EQ);
+    switch (HIPOffloadTargets.size()) {
+    default:
+      D.Diag(diag::err_drv_only_one_offload_target_supported_in) << "HIP";
+      return llvm::None;
----------------
tra wrote:
> ^^^ 
> 
> This will cause issues in practice, as we're only allowed to specify --offload once. 
> 
> I.e. we're neither allowed to override it (this goes contrary to how clang options are handled conventionally), nor can we extend or modify the list of offload variants as we can with --offload-arch.
> 
> This code initially used `getLastArg`, but that does not work for an option that controls a list of values. Perhaps we should just make `--offload=` a scalar value so it, at least, behaves consistently with other clang options.
You are right. I overlooked this part.

If multiple `--offload=` options are specified, they are supposed to be unioned. Since currently `--offload=` only accepts `amdgcn-amd-amdhsa` and `spirv64`, and they are mutually exclusive. I think it is OK.

In the future, `--offload=` will accept GPU archs, then this part needs to allow multiple `--offload=` options and more sophisticated compatibility check between different options.

I agree letting `--offload=` accept scalar value seems a good choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110622