[Clang] Simplify specifying passes via -Xoffload-linker

[jdenny-ornl]

Make it possible to do things like the following, regardless of whether the offload target is nvptx or amdgpu:

$ clang -O1 -g -fopenmp --offload-arch=native test.c                       \
    -Xoffload-linker -mllvm=-pass-remarks=inline                           \
    -Xoffload-linker -mllvm=-force-remove-attribute=g.internalized:noinline\
    -Xoffload-linker --lto-newpm-passes='forceattrs,default<O1>'           \
    -Xoffload-linker --lto-debug-pass-manager                              \
    -foffload-lto

To accomplish that:

• In clang-linker-wrapper, do not forward options via -Wl if they might have literal commas. Use -Xlinker instead.
• In clang-nvlink-wrapper, accept --lto-debug-pass-manager and --lto-newpm-passes.
• In clang-nvlink-wrapper, drop -passes because it's inconsistent with the interface of lld, which is used instead of clang-nvlink-wrapper when the target is amdgpu. Without this patch, -passes is passed to nvlink, producing an error anyway.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Joel E. Denny (jdenny-ornl)

Changes

Make it possible to do things like the following, regardless of whether $GPU_ARCH is for nvptx or amdgpu:

$ clang -O1 -g -fopenmp -fopenmp-targets=$GPU_ARCH test.c                  \
    -Xoffload-linker -mllvm=-pass-remarks=inline                           \
    -Xoffload-linker -mllvm=-force-remove-attribute=g.internalized:noinline\
    -Xoffload-linker --lto-newpm-passes='forceattrs,default<O1>'           \
    -Xoffload-linker --lto-debug-pass-manager                              \
    -foffload-lto

To accomplish that:

• In clang-linker-wrapper, do not forward options via -Wl if they might have literal commas. Use -Xlinker instead.
• In clang-nvlink-wrapper, accept --lto-debug-pass-manager and --lto-newpm-passes.
• In clang-nvlink-wrapper, drop -passes because it's inconsistent with the interface of lld, which is used instead of clang-nvlink-wrapper when the target is amdgpu. Without this patch, -passes is passed to nvlink, producing an error anyway.

---

Full diff: https://github.com/llvm/llvm-project/pull/102483.diff

5 Files Affected:

• (modified) clang/test/Driver/linker-wrapper.c (+12-6)
• (modified) clang/test/Driver/nvlink-wrapper.c (+8)
• (modified) clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp (+8-4)
• (modified) clang/tools/clang-nvlink-wrapper/ClangNVLinkWrapper.cpp (+2-13)
• (modified) clang/tools/clang-nvlink-wrapper/NVLinkOpts.td (+8)

diff --git a/clang/test/Driver/linker-wrapper.c b/clang/test/Driver/linker-wrapper.c
index 99cfdb9ebfc7c..e70715d2a9bd7 100644
--- a/clang/test/Driver/linker-wrapper.c
+++ b/clang/test/Driver/linker-wrapper.c
@@ -238,9 +238,15 @@ __attribute__((visibility("protected"), used)) int x;
 // RUN: clang-offload-packager -o %t.out \
 // RUN:   --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx908
 // RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t.o -fembed-offload-object=%t.out
-// RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run --offload-opt=-pass-remarks=foo \
-// RUN:   --linker-path=/usr/bin/ld %t.o -o a.out 2>&1 | FileCheck %s --check-prefix=OFFLOAD-OPT
-// RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run -mllvm -pass-remarks=foo \
-// RUN:   --linker-path=/usr/bin/ld %t.o -o a.out 2>&1 | FileCheck %s --check-prefix=OFFLOAD-OPT
-
-// OFFLOAD-OPT: clang{{.*}}-Wl,--plugin-opt=-pass-remarks=foo
+// RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run \
+// RUN:   --offload-opt=-pass-remarks=foo,bar --linker-path=/usr/bin/ld \
+// RUN:   %t.o -o a.out 2>&1 | FileCheck %s --check-prefix=OFFLOAD-OPT
+// RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run \
+// RUN:   -mllvm -pass-remarks=foo,bar --linker-path=/usr/bin/ld \
+// RUN:   %t.o -o a.out 2>&1 | FileCheck %s --check-prefix=MLLVM
+
+//            MLLVM: clang{{.*}}-Xlinker --plugin-opt=-pass-remarks=foo,bar
+//      OFFLOAD-OPT: clang{{.*}}-Xlinker --plugin-opt=-pass-remarks=foo,bar
+//       MLLVM-SAME: -Xlinker -mllvm=-pass-remarks=foo,bar
+//  OFFLOAD-OPT-NOT: -Xlinker -mllvm=-pass-remarks=foo,bar
+// OFFLOAD-OPT-SAME: {{$}}
diff --git a/clang/test/Driver/nvlink-wrapper.c b/clang/test/Driver/nvlink-wrapper.c
index 318315ddaca34..5d835d8d6cb2a 100644
--- a/clang/test/Driver/nvlink-wrapper.c
+++ b/clang/test/Driver/nvlink-wrapper.c
@@ -70,3 +70,11 @@ int baz() { return y + x; }
 // RUN: clang-nvlink-wrapper --dry-run %t.o %t-u.o %t-y.a \
 // RUN:   -arch sm_52 --cuda-path/opt/cuda -o a.out 2>&1 | FileCheck %s --check-prefix=PATH
 // PATH-NOT: --cuda-path=/opt/cuda
+
+//
+// Check that passes can be specified and debugged.
+//
+// RUN: clang-nvlink-wrapper --dry-run %t.o %t-u.o %t-y.a \
+// RUN:   --lto-debug-pass-manager --lto-newpm-passes=forceattrs \
+// RUN:   -arch sm_52 -o a.out 2>&1 | FileCheck %s --check-prefix=PASSES
+// PASSES: Running pass: ForceFunctionAttrsPass
diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index 24cc4f0eeadf9..7030556ce01b1 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -527,9 +527,11 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args) {
 
   // Forward all of the `--offload-opt` and similar options to the device.
   if (linkerSupportsLTO(Args)) {
-    for (auto &Arg : Args.filtered(OPT_offload_opt_eq_minus, OPT_mllvm))
+    for (auto &Arg : Args.filtered(OPT_offload_opt_eq_minus, OPT_mllvm)) {
+      CmdArgs.push_back(Args.MakeArgString("-Xlinker"));
       CmdArgs.push_back(
-          Args.MakeArgString("-Wl,--plugin-opt=" + StringRef(Arg->getValue())));
+          Args.MakeArgString("--plugin-opt=" + StringRef(Arg->getValue())));
+    }
   }
 
   if (!Triple.isNVPTX())
@@ -571,9 +573,11 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args) {
   }
 
   // Pass on -mllvm options to the linker invocation.
-  for (const opt::Arg *Arg : Args.filtered(OPT_mllvm))
+  for (const opt::Arg *Arg : Args.filtered(OPT_mllvm)) {
+    CmdArgs.push_back(Args.MakeArgString("-Xlinker"));
     CmdArgs.push_back(
-        Args.MakeArgString("-Wl,-mllvm=" + StringRef(Arg->getValue())));
+        Args.MakeArgString("-mllvm=" + StringRef(Arg->getValue())));
+  }
 
   if (Args.hasArg(OPT_debug))
     CmdArgs.push_back("-g");
diff --git a/clang/tools/clang-nvlink-wrapper/ClangNVLinkWrapper.cpp b/clang/tools/clang-nvlink-wrapper/ClangNVLinkWrapper.cpp
index 7851414ba7f4d..871fe5e4553cc 100644
--- a/clang/tools/clang-nvlink-wrapper/ClangNVLinkWrapper.cpp
+++ b/clang/tools/clang-nvlink-wrapper/ClangNVLinkWrapper.cpp
@@ -84,18 +84,6 @@ static cl::list<std::string>
     PassPlugins("load-pass-plugin",
                 cl::desc("Load passes from plugin library"));
 
-static cl::opt<std::string> PassPipeline(
-    "passes",
-    cl::desc(
-        "A textual description of the pass pipeline. To have analysis passes "
-        "available before a certain pass, add 'require<foo-analysis>'. "
-        "'-passes' overrides the pass pipeline (but not all effects) from "
-        "specifying '--opt-level=O?' (O2 is the default) to "
-        "clang-linker-wrapper.  Be sure to include the corresponding "
-        "'default<O?>' in '-passes'."));
-static cl::alias PassPipeline2("p", cl::aliasopt(PassPipeline),
-                               cl::desc("Alias for -passes"));
-
 static void printVersion(raw_ostream &OS) {
   OS << clang::getClangToolFullVersion("clang-nvlink-wrapper") << '\n';
 }
@@ -365,8 +353,9 @@ Expected<std::unique_ptr<lto::LTO>> createLTO(const ArgList &Args) {
   Conf.OptLevel = Args.getLastArgValue(OPT_O, "2")[0] - '0';
   Conf.DefaultTriple = Triple.getTriple();
 
-  Conf.OptPipeline = PassPipeline;
+  Conf.OptPipeline = Args.getLastArgValue(OPT_lto_newpm_passes, "");
   Conf.PassPlugins = PassPlugins;
+  Conf.DebugPassManager = Args.hasArg(OPT_lto_debug_pass_manager);
 
   Conf.DiagHandler = diagnosticHandler;
   Conf.CGFileType = CodeGenFileType::AssemblyFile;
diff --git a/clang/tools/clang-nvlink-wrapper/NVLinkOpts.td b/clang/tools/clang-nvlink-wrapper/NVLinkOpts.td
index 01bd0f85b1a33..a97190b7a614c 100644
--- a/clang/tools/clang-nvlink-wrapper/NVLinkOpts.td
+++ b/clang/tools/clang-nvlink-wrapper/NVLinkOpts.td
@@ -1,3 +1,6 @@
+// We try to create options similar to lld's.  That way, options passed to clang
+// -Xoffload-linker can be the same whether offloading to nvptx or amdgpu.
+
 include "llvm/Option/OptParser.td"
 
 def WrapperOnlyOption : OptionFlag;
@@ -70,6 +73,11 @@ def plugin_opt : Joined<["--", "-"], "plugin-opt=">, Flags<[WrapperOnlyOption]>,
   HelpText<"Options passed to LLVM, not including the Clang invocation. Use "
            "'--plugin-opt=--help' for a list of options.">;
 
+def lto_newpm_passes : Joined<["--"], "lto-newpm-passes=">,
+  Flags<[WrapperOnlyOption]>, HelpText<"Passes to run during LTO">;
+def lto_debug_pass_manager : Flag<["--"], "lto-debug-pass-manager">,
+  Flags<[WrapperOnlyOption]>,   HelpText<"Debug new pass manager">;
+
 def save_temps : Flag<["--", "-"], "save-temps">,
   Flags<[WrapperOnlyOption]>, HelpText<"Save intermediate results">;
 

[jhuber6]

Thanks! One nit. Also, you could use native instead of GPU_ARCH in the above ;).

[jdenny-ornl]

Also, you could use native instead of GPU_ARCH in the above ;).

Good point. I should have gotten in that habit before now.

[jhuber6]

LG, thanks

[jdenny-ornl]

Thanks for the review.