[PATCH] D111488: [Clang][clang-nvlink-wrapper] Pass nvlink path to the wrapper

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:620
+      Args.MakeArgString(Twine("--nvlink-command=" + NvlinkBin));
+  CmdArgs.push_back(NvlinkPath);
+
----------------
Nit: the variables above are used only once. It would be fine to just `push_back(MakeArgString(GetProgramPath()))`.



================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:50
+
+static cl::opt<std::string> NvlinkUserPath("nvlink-command",
+                                           cl::desc("path of nvlink binary"),
----------------
Nit. Clang already has `--ptxas-path=` option. It may be worth using `-path` suffix here for consistency, too.


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:169-170
 
-  for (size_t i = 1; i < Argv.size(); ++i) {
-    std::string Arg = Argv[i];
+  for (size_t i = 0; i < NVArgs.size(); ++i) {
+    std::string Arg = NVArgs[i];
     if (sys::path::extension(Arg) == ".a") {
----------------
Meinersbur wrote:
> 
Nit: `for (const std::string &Arg : NVArgs)` to avoid unnecessary copies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488