[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/include/clang/Basic/Cuda.h:105-107
+constexpr CudaArch DefaultCudaArch = CudaArch::SM_35;
+constexpr CudaArch DefaultHIPArch = CudaArch::GFX803;
+
----------------
Nit: those could be just enum values.
```
  ...
  LAST,
  DefaultCudaArch = SM_35,
  DefaultHIPArch = GFX803,
};
```


================
Comment at: clang/lib/Driver/Driver.cpp:4219
+    for (unsigned I = 0, E = TCAndArchs.size(); I != E; ++I)
       DeviceActions.push_back(C.MakeAction<InputAction>(*InputArg, InputType));
 
----------------
This loop can be folded into the loop above.

Alternatively, for simple loops like this one could use `llvm::for_each`.


================
Comment at: clang/lib/Driver/Driver.cpp:4244
           A = C.MakeAction<OffloadAction>(HDep, DDep);
+          ++TCAndArch;
+        } else if (isa<AssembleJobAction>(A) && Kind == Action::OFK_Cuda) {
----------------
Could you elaborate why TCAndArch is incremented only here? Don't other cases need to advance it, too? At the very least we need some comments here and for the loop in general, describing what's going on.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6998
+      // The CUDA toolchain should have a bound arch appended to the filename.
+      StringRef Arch = File.rsplit(".").first.rsplit('-').second;
+      CmdArgs.push_back(Args.MakeArgString(
----------------
Extracting arch name from the file name looks... questionable. Where does that file name come from? Can't we get this information directly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120272