[PATCH] D120272: [CUDA] Add driver support for compiling CUDA with the new driver

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:4107
+                         options::OPT_no_offload_arch_EQ)) {
+    C.getDriver().Diag(diag::err_opt_not_valid_with_opt) << "--offload-arch"
+                                                         << "--offload";
----------------
Nit: It would be nice to report specific option which triggered the diag. Reporting `--offload-arch` when it's triggered by `--no-offload-arch` would be somewhat confusing.

`Args.hasArgNoClaim(options::OPT_offload_arch_EQ) ? "--offload-arch" : --no-offload-arch` ?


================
Comment at: clang/lib/Driver/Driver.cpp:4132-4134
+      Archs.insert(CudaArchToString(CudaArch::SM_35));
+    else if (Kind == Action::OFK_HIP)
+      Archs.insert(CudaArchToString(CudaArch::GFX803));
----------------
If we do not have constants for the default CUDA/HIP arch yet, we should probably add them and use them here.


================
Comment at: clang/lib/Driver/Driver.cpp:4171
 
-    for (unsigned I = 0; I < ToolChains.size(); ++I)
+    if (!Relocatable)
+      Diags.Report(diag::err_drv_non_relocatable);
----------------
If we do not allow non-relocatable compilation with the new driver, perhaps we should make `-fgpu-rdc` enabled by default with the new driver and error out if someone attempts to use `-fno-gpu-rdc`. Otherwise we're virtually guaranteed that everyone attempting to use `-foffload-new-driver` will hit this error.



================
Comment at: clang/lib/Driver/Driver.cpp:4172
+    if (!Relocatable)
+      Diags.Report(diag::err_drv_non_relocatable);
+
----------------
Do we want to return early here?


================
Comment at: clang/lib/Driver/Driver.cpp:4221
+    auto TCAndArch = TCAndArchs.begin();
     for (Action *A : DeviceActions) {
+      DDeps.add(*A, *TCAndArch->first, TCAndArch->second.data(), Kind);
----------------
Nit: We do have `llvm::zip` for iterating over multiple containers. However, unpacking loop variable results maybe more trouble than it's worth it in such a small loop, Up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120272