From 9e8a8e78f3014324d8aa35dd1615b3f5720a5cb4 Mon Sep 17 00:00:00 2001 From: "Joel E. Denny" Date: Wed, 7 Aug 2024 19:10:33 -0400 Subject: [PATCH 1/3] [Clang] Simplify specifying passes via -Xoffload-linker 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' \ -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. --- clang/test/Driver/linker-wrapper.c | 18 ++++++++++++------ clang/test/Driver/nvlink-wrapper.c | 8 ++++++++ .../ClangLinkerWrapper.cpp | 12 ++++++++---- .../ClangNVLinkWrapper.cpp | 15 ++------------- clang/tools/clang-nvlink-wrapper/NVLinkOpts.td | 8 ++++++++ 5 files changed, 38 insertions(+), 23 deletions(-) diff --git a/clang/test/Driver/linker-wrapper.c b/clang/test/Driver/linker-wrapper.c index 99cfdb9ebfc7cd..e70715d2a9bd7e 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 318315ddaca340..5d835d8d6cb2a2 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 24cc4f0eeadf91..7030556ce01b19 100644 --- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp +++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp @@ -527,9 +527,11 @@ Expected clang(ArrayRef 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 clang(ArrayRef 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 7851414ba7f4dd..871fe5e4553ccb 100644 --- a/clang/tools/clang-nvlink-wrapper/ClangNVLinkWrapper.cpp +++ b/clang/tools/clang-nvlink-wrapper/ClangNVLinkWrapper.cpp @@ -84,18 +84,6 @@ static cl::list PassPlugins("load-pass-plugin", cl::desc("Load passes from plugin library")); -static cl::opt PassPipeline( - "passes", - cl::desc( - "A textual description of the pass pipeline. To have analysis passes " - "available before a certain pass, add 'require'. " - "'-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' 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> 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 01bd0f85b1a33e..a97190b7a614cc 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">; From d7dc3e2c6b175b804ef129a5faad9f7d15e71240 Mon Sep 17 00:00:00 2001 From: "Joel E. Denny" Date: Thu, 8 Aug 2024 11:17:18 -0400 Subject: [PATCH 2/3] Apply reviewer suggestion --- .../clang-linker-wrapper/ClangLinkerWrapper.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp index 7030556ce01b19..142c55ddd2898b 100644 --- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp +++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp @@ -527,11 +527,10 @@ Expected clang(ArrayRef 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)) { - CmdArgs.push_back(Args.MakeArgString("-Xlinker")); - CmdArgs.push_back( - Args.MakeArgString("--plugin-opt=" + StringRef(Arg->getValue()))); - } + for (auto &Arg : Args.filtered(OPT_offload_opt_eq_minus, OPT_mllvm)) + CmdArgs.append( + {"-Xlinker", + Args.MakeArgString("--plugin-opt=" + StringRef(Arg->getValue()))}); } if (!Triple.isNVPTX()) @@ -574,9 +573,8 @@ Expected clang(ArrayRef InputFiles, const ArgList &Args) { // Pass on -mllvm options to the linker invocation. for (const opt::Arg *Arg : Args.filtered(OPT_mllvm)) { - CmdArgs.push_back(Args.MakeArgString("-Xlinker")); - CmdArgs.push_back( - Args.MakeArgString("-mllvm=" + StringRef(Arg->getValue()))); + CmdArgs.append({"-Xlinker", Args.MakeArgString( + "-mllvm=" + StringRef(Arg->getValue()))}); } if (Args.hasArg(OPT_debug)) From 670e4f03cb554531681022dc92992fc8ed288f03 Mon Sep 17 00:00:00 2001 From: "Joel E. Denny" Date: Thu, 8 Aug 2024 13:08:49 -0400 Subject: [PATCH 3/3] Apply reviewer suggestion Co-authored-by: Joseph Huber --- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp index 142c55ddd2898b..52e6809a122706 100644 --- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp +++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp @@ -572,10 +572,9 @@ Expected clang(ArrayRef 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.append({"-Xlinker", Args.MakeArgString( "-mllvm=" + StringRef(Arg->getValue()))}); - } if (Args.hasArg(OPT_debug)) CmdArgs.push_back("-g");