[PATCH] D129655: [CUDA] Allow the new driver to compile CUDA in non-RDC mode

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 marked an inline comment as done.
jhuber6 added a comment.

It's also worth noting that this doesn't include the `PTX` output for JIT in the fatbinary, it would be relatively easy to include that but I wanted to ask how we should handle that.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6999
+    if (IsRDCMode)
+      CmdArgs.push_back("-fgpu-rdc");
+  } else if (IsCuda && !HostOffloadingInputs.empty() && !IsRDCMode) {
----------------
tra wrote:
> This should not be necessary -- we've already added it for all CUDA/HIP compilations above.
What I figured, I think I'm just going to prune it in a separate commit to clean this up.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7003
+    CmdArgs.push_back(HostOffloadingInputs.front().getFilename());
+  } else {
+    for (const InputInfo Input : HostOffloadingInputs)
----------------
tra wrote:
> I'm not sure I understand when we'd end up in this "else" branch.
> Perhaps the whole sequence could be restructured this way:
> 
> ```
> if ((IsCuda || IsHIP) && CudaDeviceInput) { // old-style embedding
>     CmdArgs.push_back("-fcuda-include-gpubinary");
>     CmdArgs.push_back(CudaDeviceInput->getFilename());
> } else if (!HostOffloadingInputs.empty()) { // new-driver offloading mode
>   if (Cuda && !IsRDCMode) {
>     assert(HostOffloadingInputs.size() == 1); // We should have only one GPU fatbinary to embed. Uses old-style embedding.
>     CmdArgs.push_back("-fcuda-include-gpubinary");
>     CmdArgs.push_back(HostOffloadingInputs.front().getFilename());
>   } else { // new driver embedding of GPU object files.
>      for (const InputInfo Input : HostOffloadingInputs)
>         CmdArgs.push_back(Args.MakeArgString("-fembed-offload-object=" +
>                                              TC.getInputFilename(Input)));
>   }
> }
> ```
> 
This is clearer, I'll change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129655