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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/include/clang/Basic/Cuda.h:105-107
+constexpr CudaArch DefaultCudaArch = CudaArch::SM_35;
+constexpr CudaArch DefaultHIPArch = CudaArch::GFX803;
+
----------------
tra wrote:
> Nit: those could be just enum values.
> ```
>   ...
>   LAST,
>   DefaultCudaArch = SM_35,
>   DefaultHIPArch = GFX803,
> };
> ```
Will do.


================
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));
 
----------------
tra wrote:
> This loop can be folded into the loop above.
> 
> Alternatively, for simple loops like this one could use `llvm::for_each`.
It could, I think the intention is clearer (i.e. make an input for every toolchain and architecture) having them separate. But I can merge them if that's better.


================
Comment at: clang/lib/Driver/Driver.cpp:4244
           A = C.MakeAction<OffloadAction>(HDep, DDep);
+          ++TCAndArch;
+        } else if (isa<AssembleJobAction>(A) && Kind == Action::OFK_Cuda) {
----------------
tra wrote:
> 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.
It shouldn't be, I forgot to move this out of the conditional once I added more things, I'll explain the usage as well.


================
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(
----------------
tra wrote:
> Extracting arch name from the file name looks... questionable. Where does that file name come from? Can't we get this information directly?
Yeah, it's not ideal but it was the easiest way to do it. The alternative is to find a way to traverse the job action list and find the nodes with a bound architecture set and hope they're in the same order. I can try to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120272