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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Thanks, this fixed the errors introduced in D105191 
: 
http://meinersbur.de:8011/#/builders/1/builds/1594


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:617
+  // Find nvlink and pass it as "--nvlink-command=" argument of 
clang-nvlink-wrapper.
+  auto NvlinkBin = getToolChain().GetProgramPath("nvlink");
+  const char *NvlinkPath =

Meinersbur wrote:
> [style] [[ 
> https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
>  | LLVM's coding standard does not use almost-always-auto ]].
> 
> It's not immediately obvious here, does `GetProgramPath` look into the 
> BinPath detected by CudaInstallationDetector? I applied the patch locally to 
> http://meinersbur.de:8011/#/builders/1 and it actually does work.
Yes, you are right. `CudaToolChain` constructor initializes binary paths 
obtained from `CudaInstallationDetector` which is used by this call.



Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:50
+
+static cl::opt NvlinkUserPath("nvlink-command",
+   cl::desc("path of nvlink binary"),

tra wrote:
> Nit. Clang already has `--ptxas-path=` option. It may be worth using `-path` 
> suffix here for consistency, too.
changed it to --nvlink-path


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saiyedul Islam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf56548829c4c: [Clang][clang-nvlink-wrapper] Pass nvlink path 
to the wrapper (authored by saiislam).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp

Index: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
===
--- clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
+++ clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
@@ -25,6 +25,7 @@
 /// 2. nvlink -o a.out-openmp-nvptx64 /tmp/a.cubin /tmp/b.cubin
 //===-===//
 
+#include "clang/Basic/Version.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
@@ -41,6 +42,19 @@
 
 static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
+// Mark all our options with this category, everything else (except for -help)
+// will be hidden.
+static cl::OptionCategory
+ClangNvlinkWrapperCategory("clang-nvlink-wrapper options");
+
+static cl::opt NvlinkUserPath("nvlink-path",
+   cl::desc("Path of nvlink binary"),
+   cl::cat(ClangNvlinkWrapperCategory));
+
+// Do not parse nvlink options
+static cl::list
+NVArgs(cl::Sink, cl::desc("..."));
+
 static Error runNVLink(std::string NVLinkPath,
SmallVectorImpl ) {
   std::vector NVLArgs;
@@ -119,8 +133,20 @@
   return Error::success();
 }
 
+static void PrintVersion(raw_ostream ) {
+  OS << clang::getClangToolFullVersion("clang-nvlink-wrapper") << '\n';
+}
+
 int main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
+  cl::SetVersionPrinter(PrintVersion);
+  cl::HideUnrelatedOptions(ClangNvlinkWrapperCategory);
+  cl::ParseCommandLineOptions(
+  argc, argv,
+  "A wrapper tool over nvlink program. It transparently passes every \n"
+  "input option and objects to nvlink except archive files and path of \n"
+  "nvlink binary. It reads each input archive file to extract archived \n"
+  "cubin files as temporary files.\n");
 
   if (Help) {
 cl::PrintHelpMessage();
@@ -132,12 +158,7 @@
 exit(1);
   };
 
-  ErrorOr NvlinkPath = sys::findProgramByName("nvlink");
-  if (!NvlinkPath) {
-reportError(createStringError(NvlinkPath.getError(),
-  "unable to find 'nvlink' in path"));
-  }
-
+  std::string NvlinkPath;
   SmallVector Argv(argv, argv + argc);
   SmallVector ArgvSubst;
   SmallVector TmpFiles;
@@ -145,8 +166,7 @@
   StringSaver Saver(Alloc);
   cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
 
-  for (size_t i = 1; i < Argv.size(); ++i) {
-std::string Arg = Argv[i];
+  for (const std::string  : NVArgs) {
 if (sys::path::extension(Arg) == ".a") {
   if (Error Err = extractArchiveFiles(Arg, ArgvSubst, TmpFiles))
 reportError(std::move(Err));
@@ -155,7 +175,19 @@
 }
   }
 
-  if (Error Err = runNVLink(NvlinkPath.get(), ArgvSubst))
+  NvlinkPath = NvlinkUserPath;
+
+  // If user hasn't specified nvlink binary then search it in PATH
+  if (NvlinkPath.empty()) {
+ErrorOr NvlinkPathErr = sys::findProgramByName("nvlink");
+if (!NvlinkPathErr) {
+  reportError(createStringError(NvlinkPathErr.getError(),
+"unable to find 'nvlink' in path"));
+}
+NvlinkPath = NvlinkPathErr.get();
+  }
+
+  if (Error Err = runNVLink(NvlinkPath, ArgvSubst))
 reportError(std::move(Err));
   if (Error Err = cleanupTmpFiles(TmpFiles))
 reportError(std::move(Err));
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -613,6 +613,11 @@
   AddStaticDeviceLibsLinking(C, *this, JA, Inputs, Args, CmdArgs, "nvptx", GPUArch,
   false, false);
 
+  // Find nvlink and pass it as "--nvlink-path=" argument of
+  // clang-nvlink-wrapper.
+  CmdArgs.push_back(Args.MakeArgString(
+  Twine("--nvlink-path=" + getToolChain().GetProgramPath("nvlink";
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath("clang-nvlink-wrapper"));
   C.addCommand(std::make_unique(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam updated this revision to Diff 378979.
saiislam added a comment.

clang-format(ed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp

Index: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
===
--- clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
+++ clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
@@ -25,6 +25,7 @@
 /// 2. nvlink -o a.out-openmp-nvptx64 /tmp/a.cubin /tmp/b.cubin
 //===-===//
 
+#include "clang/Basic/Version.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
@@ -41,6 +42,19 @@
 
 static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
+// Mark all our options with this category, everything else (except for -help)
+// will be hidden.
+static cl::OptionCategory
+ClangNvlinkWrapperCategory("clang-nvlink-wrapper options");
+
+static cl::opt NvlinkUserPath("nvlink-path",
+   cl::desc("Path of nvlink binary"),
+   cl::cat(ClangNvlinkWrapperCategory));
+
+// Do not parse nvlink options
+static cl::list
+NVArgs(cl::Sink, cl::desc("..."));
+
 static Error runNVLink(std::string NVLinkPath,
SmallVectorImpl ) {
   std::vector NVLArgs;
@@ -119,8 +133,20 @@
   return Error::success();
 }
 
+static void PrintVersion(raw_ostream ) {
+  OS << clang::getClangToolFullVersion("clang-nvlink-wrapper") << '\n';
+}
+
 int main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
+  cl::SetVersionPrinter(PrintVersion);
+  cl::HideUnrelatedOptions(ClangNvlinkWrapperCategory);
+  cl::ParseCommandLineOptions(
+  argc, argv,
+  "A wrapper tool over nvlink program. It transparently passes every \n"
+  "input option and objects to nvlink except archive files and path of \n"
+  "nvlink binary. It reads each input archive file to extract archived \n"
+  "cubin files as temporary files.\n");
 
   if (Help) {
 cl::PrintHelpMessage();
@@ -132,12 +158,7 @@
 exit(1);
   };
 
-  ErrorOr NvlinkPath = sys::findProgramByName("nvlink");
-  if (!NvlinkPath) {
-reportError(createStringError(NvlinkPath.getError(),
-  "unable to find 'nvlink' in path"));
-  }
-
+  std::string NvlinkPath;
   SmallVector Argv(argv, argv + argc);
   SmallVector ArgvSubst;
   SmallVector TmpFiles;
@@ -145,8 +166,7 @@
   StringSaver Saver(Alloc);
   cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
 
-  for (size_t i = 1; i < Argv.size(); ++i) {
-std::string Arg = Argv[i];
+  for (const std::string  : NVArgs) {
 if (sys::path::extension(Arg) == ".a") {
   if (Error Err = extractArchiveFiles(Arg, ArgvSubst, TmpFiles))
 reportError(std::move(Err));
@@ -155,7 +175,19 @@
 }
   }
 
-  if (Error Err = runNVLink(NvlinkPath.get(), ArgvSubst))
+  NvlinkPath = NvlinkUserPath;
+
+  // If user hasn't specified nvlink binary then search it in PATH
+  if (NvlinkPath.empty()) {
+ErrorOr NvlinkPathErr = sys::findProgramByName("nvlink");
+if (!NvlinkPathErr) {
+  reportError(createStringError(NvlinkPathErr.getError(),
+"unable to find 'nvlink' in path"));
+}
+NvlinkPath = NvlinkPathErr.get();
+  }
+
+  if (Error Err = runNVLink(NvlinkPath, ArgvSubst))
 reportError(std::move(Err));
   if (Error Err = cleanupTmpFiles(TmpFiles))
 reportError(std::move(Err));
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -613,6 +613,11 @@
   AddStaticDeviceLibsLinking(C, *this, JA, Inputs, Args, CmdArgs, "nvptx", GPUArch,
   false, false);
 
+  // Find nvlink and pass it as "--nvlink-path=" argument of
+  // clang-nvlink-wrapper.
+  CmdArgs.push_back(Args.MakeArgString(
+  Twine("--nvlink-path=" + getToolChain().GetProgramPath("nvlink";
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath("clang-nvlink-wrapper"));
   C.addCommand(std::make_unique(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam updated this revision to Diff 378978.
saiislam marked 5 inline comments as done.
saiislam added a comment.

Thanks, Michael and Artem!

1. Changed nvlink-command to nvlink-path.
2. Answered queries and done refactoring as suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp

Index: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
===
--- clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
+++ clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
@@ -25,6 +25,7 @@
 /// 2. nvlink -o a.out-openmp-nvptx64 /tmp/a.cubin /tmp/b.cubin
 //===-===//
 
+#include "clang/Basic/Version.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
@@ -41,6 +42,19 @@
 
 static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
+// Mark all our options with this category, everything else (except for -help)
+// will be hidden.
+static cl::OptionCategory
+ClangNvlinkWrapperCategory("clang-nvlink-wrapper options");
+
+static cl::opt NvlinkUserPath("nvlink-path",
+   cl::desc("Path of nvlink binary"),
+   cl::cat(ClangNvlinkWrapperCategory));
+
+// Do not parse nvlink options
+static cl::list
+NVArgs(cl::Sink, cl::desc("..."));
+
 static Error runNVLink(std::string NVLinkPath,
SmallVectorImpl ) {
   std::vector NVLArgs;
@@ -119,8 +133,20 @@
   return Error::success();
 }
 
+static void PrintVersion(raw_ostream ) {
+  OS << clang::getClangToolFullVersion("clang-nvlink-wrapper") << '\n';
+}
+
 int main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
+  cl::SetVersionPrinter(PrintVersion);
+  cl::HideUnrelatedOptions(ClangNvlinkWrapperCategory);
+  cl::ParseCommandLineOptions(
+  argc, argv,
+  "A wrapper tool over nvlink program. It transparently passes every \n"
+  "input option and objects to nvlink except archive files and path of \n"
+  "nvlink binary. It reads each input archive file to extract archived \n"
+  "cubin files as temporary files.\n");
 
   if (Help) {
 cl::PrintHelpMessage();
@@ -132,12 +158,7 @@
 exit(1);
   };
 
-  ErrorOr NvlinkPath = sys::findProgramByName("nvlink");
-  if (!NvlinkPath) {
-reportError(createStringError(NvlinkPath.getError(),
-  "unable to find 'nvlink' in path"));
-  }
-
+  std::string NvlinkPath;
   SmallVector Argv(argv, argv + argc);
   SmallVector ArgvSubst;
   SmallVector TmpFiles;
@@ -145,8 +166,7 @@
   StringSaver Saver(Alloc);
   cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
 
-  for (size_t i = 1; i < Argv.size(); ++i) {
-std::string Arg = Argv[i];
+  for (const std::string  : NVArgs) {
 if (sys::path::extension(Arg) == ".a") {
   if (Error Err = extractArchiveFiles(Arg, ArgvSubst, TmpFiles))
 reportError(std::move(Err));
@@ -155,7 +175,19 @@
 }
   }
 
-  if (Error Err = runNVLink(NvlinkPath.get(), ArgvSubst))
+  NvlinkPath = NvlinkUserPath;
+
+  // If user hasn't specified nvlink binary then search it in PATH
+  if (NvlinkPath.empty()) {
+ErrorOr NvlinkPathErr = sys::findProgramByName("nvlink");
+if (!NvlinkPathErr) {
+  reportError(createStringError(NvlinkPathErr.getError(),
+"unable to find 'nvlink' in path"));
+}
+NvlinkPath = NvlinkPathErr.get();
+  }
+
+  if (Error Err = runNVLink(NvlinkPath, ArgvSubst))
 reportError(std::move(Err));
   if (Error Err = cleanupTmpFiles(TmpFiles))
 reportError(std::move(Err));
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -613,6 +613,10 @@
   AddStaticDeviceLibsLinking(C, *this, JA, Inputs, Args, CmdArgs, "nvptx", GPUArch,
   false, false);
 
+  // Find nvlink and pass it as "--nvlink-path=" argument of
+  // clang-nvlink-wrapper.
+  CmdArgs.push_back(Args.MakeArgString(Twine("--nvlink-path=" + getToolChain().GetProgramPath("nvlink";
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath("clang-nvlink-wrapper"));
   C.addCommand(std::make_unique(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 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  : 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

Some nitpicks, but it fixes the build, so LGTM.




Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:617
+  // Find nvlink and pass it as "--nvlink-command=" argument of 
clang-nvlink-wrapper.
+  auto NvlinkBin = getToolChain().GetProgramPath("nvlink");
+  const char *NvlinkPath =

[style] [[ 
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
 | LLVM's coding standard does not use almost-always-auto ]].

It's not immediately obvious here, does `GetProgramPath` look into the BinPath 
detected by CudaInstallationDetector? I applied the patch locally to 
http://meinersbur.de:8011/#/builders/1 and it actually does work.



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") {




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam updated this revision to Diff 378633.
saiislam added a comment.

Fixed typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp

Index: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
===
--- clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
+++ clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
@@ -25,6 +25,7 @@
 /// 2. nvlink -o a.out-openmp-nvptx64 /tmp/a.cubin /tmp/b.cubin
 //===-===//
 
+#include "clang/Basic/Version.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
@@ -41,6 +42,19 @@
 
 static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
+// Mark all our options with this category, everything else (except for -help)
+// will be hidden.
+static cl::OptionCategory
+ClangNvlinkWrapperCategory("clang-nvlink-wrapper options");
+
+static cl::opt NvlinkUserPath("nvlink-command",
+   cl::desc("path of nvlink binary"),
+   cl::cat(ClangNvlinkWrapperCategory));
+
+// Do not parse nvlink options
+static cl::list
+NVArgs(cl::Sink, cl::desc("..."));
+
 static Error runNVLink(std::string NVLinkPath,
SmallVectorImpl ) {
   std::vector NVLArgs;
@@ -119,8 +133,20 @@
   return Error::success();
 }
 
+static void PrintVersion(raw_ostream ) {
+  OS << clang::getClangToolFullVersion("clang-nvlink-wrapper") << '\n';
+}
+
 int main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
+  cl::SetVersionPrinter(PrintVersion);
+  cl::HideUnrelatedOptions(ClangNvlinkWrapperCategory);
+  cl::ParseCommandLineOptions(
+  argc, argv,
+  "A wrapper tool over nvlink program. It transparently passes every \n"
+  "input option and objects to nvlink except archive files and path of \n"
+  "nvlink binary. It reads each input archive file to extract archived \n"
+  "cubin files as temporary files.\n");
 
   if (Help) {
 cl::PrintHelpMessage();
@@ -132,12 +158,7 @@
 exit(1);
   };
 
-  ErrorOr NvlinkPath = sys::findProgramByName("nvlink");
-  if (!NvlinkPath) {
-reportError(createStringError(NvlinkPath.getError(),
-  "unable to find 'nvlink' in path"));
-  }
-
+  std::string NvlinkPath;
   SmallVector Argv(argv, argv + argc);
   SmallVector ArgvSubst;
   SmallVector TmpFiles;
@@ -145,8 +166,8 @@
   StringSaver Saver(Alloc);
   cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
 
-  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") {
   if (Error Err = extractArchiveFiles(Arg, ArgvSubst, TmpFiles))
 reportError(std::move(Err));
@@ -155,7 +176,19 @@
 }
   }
 
-  if (Error Err = runNVLink(NvlinkPath.get(), ArgvSubst))
+  NvlinkPath = NvlinkUserPath;
+
+  // If user hasn't specified nvlink binary then search it in PATH
+  if (NvlinkPath.empty()) {
+ErrorOr NvlinkPathErr = sys::findProgramByName("nvlink");
+if (!NvlinkPathErr) {
+  reportError(createStringError(NvlinkPathErr.getError(),
+"unable to find 'nvlink' in path"));
+}
+NvlinkPath = NvlinkPathErr.get();
+  }
+
+  if (Error Err = runNVLink(NvlinkPath, ArgvSubst))
 reportError(std::move(Err));
   if (Error Err = cleanupTmpFiles(TmpFiles))
 reportError(std::move(Err));
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -613,6 +613,12 @@
   AddStaticDeviceLibsLinking(C, *this, JA, Inputs, Args, CmdArgs, "nvptx", GPUArch,
   false, false);
 
+  // Find nvlink and pass it as "--nvlink-command=" argument of clang-nvlink-wrapper.
+  auto NvlinkBin = getToolChain().GetProgramPath("nvlink");
+  const char *NvlinkPath =
+  Args.MakeArgString(Twine("--nvlink-command=" + NvlinkBin));
+  CmdArgs.push_back(NvlinkPath);
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath("clang-nvlink-wrapper"));
   C.addCommand(std::make_unique(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam updated this revision to Diff 378630.
saiislam marked 4 inline comments as done.
saiislam added a comment.

1. Changed the option from path to nvlink-command.
2. Command line arguments are now parsed using proper API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp

Index: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
===
--- clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
+++ clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
@@ -25,6 +25,7 @@
 /// 2. nvlink -o a.out-openmp-nvptx64 /tmp/a.cubin /tmp/b.cubin
 //===-===//
 
+#include "clang/Basic/Version.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
@@ -41,6 +42,19 @@
 
 static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
+// Mark all our options with this category, everything else (except for -help)
+// will be hidden.
+static cl::OptionCategory
+ClangNvlinkWrapperCategory("clang-nvlink-wrapper options");
+
+static cl::opt NvlinkUserPath("nvlink-command",
+   cl::desc("path of nvlink binary"),
+   cl::cat(ClangNvlinkWrapperCategory));
+
+// Do not parse nvlink options
+static cl::list
+NVArgs(cl::Sink, cl::desc("..."));
+
 static Error runNVLink(std::string NVLinkPath,
SmallVectorImpl ) {
   std::vector NVLArgs;
@@ -119,8 +133,20 @@
   return Error::success();
 }
 
+static void PrintVersion(raw_ostream ) {
+  OS << clang::getClangToolFullVersion("clang-offload-bundler") << '\n';
+}
+
 int main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
+  cl::SetVersionPrinter(PrintVersion);
+  cl::HideUnrelatedOptions(ClangNvlinkWrapperCategory);
+  cl::ParseCommandLineOptions(
+  argc, argv,
+  "A wrapper tool over nvlink program. It transparently passes every \n"
+  "input option and objects to nvlink except archive files and path of \n"
+  "nvlink binary. It reads each input archive file to extract archived \n"
+  "cubin files as temporary files.\n");
 
   if (Help) {
 cl::PrintHelpMessage();
@@ -132,12 +158,7 @@
 exit(1);
   };
 
-  ErrorOr NvlinkPath = sys::findProgramByName("nvlink");
-  if (!NvlinkPath) {
-reportError(createStringError(NvlinkPath.getError(),
-  "unable to find 'nvlink' in path"));
-  }
-
+  std::string NvlinkPath;
   SmallVector Argv(argv, argv + argc);
   SmallVector ArgvSubst;
   SmallVector TmpFiles;
@@ -145,8 +166,8 @@
   StringSaver Saver(Alloc);
   cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
 
-  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") {
   if (Error Err = extractArchiveFiles(Arg, ArgvSubst, TmpFiles))
 reportError(std::move(Err));
@@ -155,7 +176,19 @@
 }
   }
 
-  if (Error Err = runNVLink(NvlinkPath.get(), ArgvSubst))
+  NvlinkPath = NvlinkUserPath;
+
+  // If user hasn't specified nvlink binary then search it in PATH
+  if (NvlinkPath.empty()) {
+ErrorOr NvlinkPathErr = sys::findProgramByName("nvlink");
+if (!NvlinkPathErr) {
+  reportError(createStringError(NvlinkPathErr.getError(),
+"unable to find 'nvlink' in path"));
+}
+NvlinkPath = NvlinkPathErr.get();
+  }
+
+  if (Error Err = runNVLink(NvlinkPath, ArgvSubst))
 reportError(std::move(Err));
   if (Error Err = cleanupTmpFiles(TmpFiles))
 reportError(std::move(Err));
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -613,6 +613,12 @@
   AddStaticDeviceLibsLinking(C, *this, JA, Inputs, Args, CmdArgs, "nvptx", GPUArch,
   false, false);
 
+  // Find nvlink and pass it as "--nvlink-command=" argument of clang-nvlink-wrapper.
+  auto NvlinkBin = getToolChain().GetProgramPath("nvlink");
+  const char *NvlinkPath =
+  Args.MakeArgString(Twine("--nvlink-command=" + NvlinkBin));
+  CmdArgs.push_back(NvlinkPath);
+
   const char *Exec =
   Args.MakeArgString(getToolChain().GetProgramPath("clang-nvlink-wrapper"));
   C.addCommand(std::make_unique(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.

Thanks for the path, but command line parsing should be done properly.




Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:50
+static cl::opt
+NvlinkUserPath("path", cl::desc("path of directory containing nvlink"),
+   cl::cat(ClangNvlinkWrapperCategory));

I suggest a more descriptive flag, `path` is too general, like `--nvlink` or 
`--nvlink-command` (bugpoint uses `--opt-command` for the path to `opt`)



Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:124
   sys::PrintStackTraceOnErrorSignal(argv[0]);
-
   if (Help) {

[nit] unrelated whitespace change



Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:133
   sys::PrintStackTraceOnErrorSignal(argv[0]);
-
   if (Help) {
 cl::PrintHelpMessage();

`cl::ParseCommandLineOptions(argc, argv,"Program description")` must have been 
called beforehand to make this work.



Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:154
+StringRef ArgRef(Arg);
+auto NvlPath = ArgRef.startswith_insensitive("--path=");
 if (sys::path::extension(Arg) == ".a") {

This way of parsing does not use the declared `NvlinkUserPath` option. Use 
`cl::ParseCommandLineOptions` do parse the arguments and 
```
cl::list InputFiles(cl::Positional, cl::desc("..."));
```
to catch all non-option arguments.

The way it is done here also does not catch the `--path ` (without 
equal sign) syntax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam created this revision.
saiislam added reviewers: Meinersbur, ye-luo, JonChesterfield.
saiislam requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

Added support of a "--path=" option in clang-nvlink-wrapper which
takes the path of directory containing nvlink binary.

Static Device Library support for OpenMP (D105191 
) now searches for
nvlink binary and passes its location via this option. In absence
of this option, nvlink binary is searched in locations in PATH.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D111488

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp


Index: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
===
--- clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
+++ clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
@@ -41,6 +41,15 @@
 
 static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
+// Mark all our options with this category, everything else (except for -help)
+// will be hidden.
+static cl::OptionCategory
+ClangNvlinkWrapperCategory("clang-nvlink-wrapper options");
+
+static cl::opt
+NvlinkUserPath("path", cl::desc("path of directory containing nvlink"),
+   cl::cat(ClangNvlinkWrapperCategory));
+
 static Error runNVLink(std::string NVLinkPath,
SmallVectorImpl ) {
   std::vector NVLArgs;
@@ -121,7 +130,6 @@
 
 int main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
-
   if (Help) {
 cl::PrintHelpMessage();
 return 0;
@@ -132,12 +140,7 @@
 exit(1);
   };
 
-  ErrorOr NvlinkPath = sys::findProgramByName("nvlink");
-  if (!NvlinkPath) {
-reportError(createStringError(NvlinkPath.getError(),
-  "unable to find 'nvlink' in path"));
-  }
-
+  std::string NvlinkPath;
   SmallVector Argv(argv, argv + argc);
   SmallVector ArgvSubst;
   SmallVector TmpFiles;
@@ -147,15 +150,28 @@
 
   for (size_t i = 1; i < Argv.size(); ++i) {
 std::string Arg = Argv[i];
+StringRef ArgRef(Arg);
+auto NvlPath = ArgRef.startswith_insensitive("--path=");
 if (sys::path::extension(Arg) == ".a") {
   if (Error Err = extractArchiveFiles(Arg, ArgvSubst, TmpFiles))
 reportError(std::move(Err));
+} else if (NvlPath) {
+  NvlinkPath = ArgRef.substr(7).str().append("/nvlink");
 } else {
   ArgvSubst.push_back(Arg);
 }
   }
 
-  if (Error Err = runNVLink(NvlinkPath.get(), ArgvSubst))
+  if (NvlinkPath.empty()) {
+ErrorOr NvlinkPathErr = sys::findProgramByName("nvlink");
+if (!NvlinkPathErr) {
+  reportError(createStringError(NvlinkPathErr.getError(),
+"unable to find 'nvlink' in path"));
+}
+NvlinkPath = NvlinkPathErr.get();
+  }
+
+  if (Error Err = runNVLink(NvlinkPath, ArgvSubst))
 reportError(std::move(Err));
   if (Error Err = cleanupTmpFiles(TmpFiles))
 reportError(std::move(Err));
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -613,6 +613,13 @@
   AddStaticDeviceLibsLinking(C, *this, JA, Inputs, Args, CmdArgs, "nvptx", 
GPUArch,
   false, false);
 
+  // Find nvlink and pass it as "--path=" argument of clang-nvlink-wrapper.
+  auto NvlinkDir =
+  llvm::sys::path::parent_path(getToolChain().GetProgramPath("nvlink"))
+  .str();
+  const char *NvlinkPath = Args.MakeArgString(Twine("--path=" + NvlinkDir));
+  CmdArgs.push_back(NvlinkPath);
+
   const char *Exec =
   
Args.MakeArgString(getToolChain().GetProgramPath("clang-nvlink-wrapper"));
   C.addCommand(std::make_unique(


Index: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
===
--- clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
+++ clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp
@@ -41,6 +41,15 @@
 
 static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
+// Mark all our options with this category, everything else (except for -help)
+// will be hidden.
+static cl::OptionCategory
+ClangNvlinkWrapperCategory("clang-nvlink-wrapper options");
+
+static cl::opt
+NvlinkUserPath("path", cl::desc("path of directory containing nvlink"),
+   cl::cat(ClangNvlinkWrapperCategory));
+
 static Error runNVLink(std::string NVLinkPath,
SmallVectorImpl ) {
   std::vector NVLArgs;
@@ -121,7 +130,6 @@
 
 int main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
-
   if (Help) {
 cl::PrintHelpMessage();
 return 0;
@@ -132,12 +140,7 @@
 exit(1);
   };
 
-  ErrorOr