[PATCH] D118944: [OpenMP] Add Cuda path to linker wrapper tool

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8161-8162
+      if (CudaInstallation.isValid())
+        CmdArgs.push_back(Args.MakeArgString(
+            "-cuda-path=" + CudaInstallation.getInstallPath()));
+    }
----------------
Meinersbur wrote:
> Since there is no `break`, would this potentially add multiple `--cuda-path`?
Right now the tool chains are initialized based on triples. I don't think it's technically possible for the user to perform an action like `-fopenmp-targets=nvptx64-nvidia-cuda,nvptx-nvidia-cuda`. The flag is listed `ZeroOrMore`, so I'm not sure if it's worth explicitly preventing, we'll just take the most recently passed in one.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8183
       const ToolChain *TC = I.second;
-      const Driver &D = TC->getDriver();
+      const Driver &TCDriver = TC->getDriver();
       const ArgList &TCArgs = C.getArgsForToolChain(TC, "", Action::OFK_OpenMP);
----------------
Meinersbur wrote:
> [nit] unrelated rename
Avoids shadowing the new `D` I added.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8258-8261
+  // Construct the link job so we can wrap around it.
+  Linker->ConstructJob(C, JA, Output, Inputs, Args, LinkingOutput);
+  const auto &LinkCommand = C.getJobs().getJobs().back();
+
----------------
Meinersbur wrote:
> Is moving this relevant?
No, but I wanted to move it so I figured I might as well while I'm here, similar with the other unrelated changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118944