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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D110622#3177490 <https://reviews.llvm.org/D110622#3177490>, @yaxunl wrote:

> I don't think `--offload=` is restricted to be specified only once. The test checks `--offload-arch=` and `--offload=` are mutually exclusive.

It effectively is. See my inline comment.

  // RUN: %clang -### -x hip -target x86_64-linux-gnu --offload=foo,bar \
  // RUN:   --hip-path=%S/Inputs/hipspv -nogpuinc -nogpulib %s \
  // RUN: 2>&1 | FileCheck --check-prefix=TOO-MANY-TARGETS %s
  
  // TOO-MANY-TARGETS: error: Only one offload target is supported in HIP.



================
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;
----------------
^^^ 

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110622