[LinkerWrapper] Make `-Xoffload-linker` match `-Xlinker` semantics #101032

jhuber6 · 2024-07-29T15:42:48Z

Summary:
-Xlinker is supposed to pass options to the linker, while
-Xoffload-linker instead passes it to the clang job. This is
unintuitive and results in unnecessarily complex command lines. Because
passing it to the clang job is still useful, I've added
-device-compiler. This changes the behavior of the flag, but I think
it should be fine in this case since it's likely rarely used and has a
direct replacement.

llvmbot · 2024-07-29T15:43:21Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Joseph Huber (jhuber6)

Changes

Summary:
-Xlinker is supposed to pass options to the linker, while
-Xoffload-linker instead passes it to the clang job. This is
unintuitive and results in unnecessarily complex command lines. Because
passing it to the clang job is still useful, I've added
-device-compiler. This changes the behavior of the flag, but I think
it should be fine in this case since it's likely rarely used and has a
direct replacement.

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

5 Files Affected:

(modified) clang/lib/Driver/ToolChains/Clang.cpp (+1-1)
(modified) clang/test/Driver/linker-wrapper.c (+5-5)
(modified) clang/test/Driver/openmp-offload-gpu.c (+1-1)
(modified) clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp (+19)
(modified) clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td (+6)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 6790079ccd26a..843d68c85bc59 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -9155,7 +9155,7 @@ void LinkerWrapper::ConstructJob(Compilation &C, const JobAction &JA,
   // If we disable the GPU C library support it needs to be forwarded to the
   // link job.
   if (!Args.hasFlag(options::OPT_gpulibc, options::OPT_nogpulibc, true))
-    CmdArgs.push_back("--device-linker=-nolibc");
+    CmdArgs.push_back("--device-compiler=-nolibc");
 
   // Add the linker arguments to be forwarded by the wrapper.
   CmdArgs.push_back(Args.MakeArgString(Twine("--linker-path=") +
diff --git a/clang/test/Driver/linker-wrapper.c b/clang/test/Driver/linker-wrapper.c
index f165a410c1929..99cfdb9ebfc7c 100644
--- a/clang/test/Driver/linker-wrapper.c
+++ b/clang/test/Driver/linker-wrapper.c
@@ -129,14 +129,14 @@ __attribute__((visibility("protected"), used)) int x;
 // RUN:   -fembed-offload-object=%t.out
 // RUN: clang-linker-wrapper --dry-run --host-triple=x86_64-unknown-linux-gnu \
 // RUN:   --linker-path=/usr/bin/ld --device-linker=foo=bar --device-linker=a \
-// RUN:   --device-linker=nvptx64-nvidia-cuda=b \
+// RUN:   --device-linker=nvptx64-nvidia-cuda=b --device-compiler=foo\
 // RUN:   %t.o -o a.out 2>&1 | FileCheck %s --check-prefix=LINKER-ARGS
 
-// LINKER-ARGS: clang{{.*}}--target=amdgcn-amd-amdhsa{{.*}}foo=bar{{.*}}a
-// LINKER-ARGS: clang{{.*}}--target=nvptx64-nvidia-cuda{{.*}}foo=bar{{.*}}a b
+// LINKER-ARGS: clang{{.*}}--target=amdgcn-amd-amdhsa{{.*}}-Xlinker foo=bar{{.*}}-Xlinker a{{.*}}foo
+// LINKER-ARGS: clang{{.*}}--target=nvptx64-nvidia-cuda{{.*}}-Xlinker foo=bar{{.*}}-Xlinker a -Xlinker b{{.*}}foo
 
-// RUN: not clang-linker-wrapper --dry-run --host-triple=x86_64-unknown-linux-gnu -ldummy \
-// RUN:   --linker-path=/usr/bin/ld --device-linker=a --device-linker=nvptx64-nvidia-cuda=b \
+// RUN: not clang-linker-wrapper --dry-run --host-triple=x86_64-unknown-linux-gnu \
+// RUN:   -ldummy --linker-path=/usr/bin/ld \
 // RUN:   -o a.out 2>&1 | FileCheck %s --check-prefix=MISSING-LIBRARY
 
 // MISSING-LIBRARY: error: unable to find library -ldummy
diff --git a/clang/test/Driver/openmp-offload-gpu.c b/clang/test/Driver/openmp-offload-gpu.c
index 0314f28f38a43..ef6cbdded6a6f 100644
--- a/clang/test/Driver/openmp-offload-gpu.c
+++ b/clang/test/Driver/openmp-offload-gpu.c
@@ -377,4 +377,4 @@
 // RUN:      --cuda-path=%S/Inputs/CUDA_102/usr/local/cuda \
 // RUN:      --offload-arch=sm_52 -nogpulibc -nogpuinc %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=LIBC-GPU %s
-// LIBC-GPU: clang-linker-wrapper{{.*}}"--device-linker=-nolibc"
+// LIBC-GPU: clang-linker-wrapper{{.*}}"--device-compiler=-nolibc"
diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index 2db7b6ac823ec..a7a7b9634605e 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -599,6 +599,8 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args) {
         std::back_inserter(CmdArgs));
 
   for (StringRef Arg : Args.getAllArgValues(OPT_linker_arg_EQ))
+    CmdArgs.append({"-Xlinker", Args.MakeArgString(Arg)});
+  for (StringRef Arg : Args.getAllArgValues(OPT_compiler_arg_EQ))
     CmdArgs.push_back(Args.MakeArgString(Arg));
 
   for (StringRef Arg : Args.getAllArgValues(OPT_builtin_bitcode_EQ)) {
@@ -1236,6 +1238,23 @@ DerivedArgList getLinkerArgs(ArrayRef<OffloadFile> Input,
                        Args.MakeArgString(Value));
   }
 
+  // Forward '-Xoffload-compiler' options to the appropriate backend.
+  for (StringRef Arg : Args.getAllArgValues(OPT_device_compiler_args_EQ)) {
+    auto [Triple, Value] = Arg.split('=');
+    llvm::Triple TT(Triple);
+    // If this isn't a recognized triple then it's an `arg=value` option.
+    if (TT.getArch() <= Triple::ArchType::UnknownArch ||
+        TT.getArch() > Triple::ArchType::LastArchType)
+      DAL.AddJoinedArg(nullptr, Tbl.getOption(OPT_compiler_arg_EQ),
+                       Args.MakeArgString(Arg));
+    else if (Value.empty())
+      DAL.AddJoinedArg(nullptr, Tbl.getOption(OPT_compiler_arg_EQ),
+                       Args.MakeArgString(Triple));
+    else if (Triple == DAL.getLastArgValue(OPT_triple_EQ))
+      DAL.AddJoinedArg(nullptr, Tbl.getOption(OPT_compiler_arg_EQ),
+                       Args.MakeArgString(Value));
+  }
+
   return DAL;
 }
 
diff --git a/clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td b/clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td
index a87d5ec8aa9b3..2a29bcacf428a 100644
--- a/clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td
+++ b/clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td
@@ -32,6 +32,9 @@ def builtin_bitcode_EQ : Joined<["--"], "builtin-bitcode=">,
 def device_linker_args_EQ : Joined<["--"], "device-linker=">,
   Flags<[WrapperOnlyOption]>, MetaVarName<"<value> or <triple>=<value>">,
   HelpText<"Arguments to pass to the device linker invocation">;
+def device_compiler_args_EQ : Joined<["--"], "device-compiler=">,
+  Flags<[WrapperOnlyOption]>, MetaVarName<"<value> or <triple>=<value>">,
+  HelpText<"Arguments to pass to the device linker invocation">;
 def clang_backend : Flag<["--"], "clang-backend">,
   Flags<[WrapperOnlyOption]>,
   HelpText<"Run the backend using clang rather than the LTO backend">;
@@ -91,6 +94,9 @@ def whole_program : Flag<["--"], "whole-program">,
 def linker_arg_EQ : Joined<["--"], "linker-arg=">,
   Flags<[DeviceOnlyOption, HelpHidden]>,
   HelpText<"An extra argument to be passed to the linker">;
+def compiler_arg_EQ : Joined<["--"], "compiler-arg=">,
+  Flags<[DeviceOnlyOption, HelpHidden]>,
+  HelpText<"An extra argument to be passed to the compiler">;
 
 // Arguments for the LLVM backend.
 def mllvm : Separate<["-"], "mllvm">, Flags<[WrapperOnlyOption]>,

arsenm · 2024-07-29T15:45:33Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

+    if (TT.getArch() <= Triple::ArchType::UnknownArch ||
+        TT.getArch() > Triple::ArchType::LastArchType)


I'd expect Triple to have an isvalid or operator bool or something instead of needing to do this range check

I could probably use Triple::parseArch(Str) == Triple::ArchType::UnknownArch) instead, I think that's as close as we have.

Summary: `-Xlinker` is supposed to pass options to the linker, while `-Xoffload-linker` instead passes it to the `clang` job. This is unintuitive and results in unnecessarily complex command lines. Because passing it to the clang job is still useful, I've added `-device-compiler`. This changes the behavior of the flag, but I think it should be fine in this case since it's likely rarely used and has a direct replacement.

clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td

Co-authored-by: Joel E. Denny <[email protected]>

jplehr

Do we have some sort of documentation where this change of behavior needs to be communicated?

jhuber6 · 2024-07-29T20:37:57Z

Do we have some sort of documentation where this change of behavior needs to be communicated?

Could probably specify it more in the docs.

jhuber6 requested review from arsenm, jdenny-ornl, jdoerfert, jplehr and saiislam July 29, 2024 15:42

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jul 29, 2024

arsenm reviewed Jul 29, 2024

View reviewed changes

jhuber6 force-pushed the Xoffload branch from df57d93 to 19d0d56 Compare July 29, 2024 15:53

jdenny-ornl reviewed Jul 29, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td Outdated Show resolved Hide resolved

Update clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td

7f12ad7

Co-authored-by: Joel E. Denny <[email protected]>

jdenny-ornl approved these changes Jul 29, 2024

View reviewed changes

jplehr reviewed Jul 29, 2024

View reviewed changes

jhuber6 mentioned this pull request Jul 29, 2024

Add debug options to clang-linker-wrapper #101008

Closed

jhuber6 merged commit 76605f5 into llvm:main Jul 29, 2024
7 checks passed

jhuber6 deleted the Xoffload branch October 14, 2024 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LinkerWrapper] Make `-Xoffload-linker` match `-Xlinker` semantics #101032

[LinkerWrapper] Make `-Xoffload-linker` match `-Xlinker` semantics #101032

Uh oh!

jhuber6 commented Jul 29, 2024

Uh oh!

llvmbot commented Jul 29, 2024 •

edited

Loading

Uh oh!

arsenm Jul 29, 2024

Uh oh!

jhuber6 Jul 29, 2024

Uh oh!

Uh oh!

jplehr left a comment

Uh oh!

jhuber6 commented Jul 29, 2024

Uh oh!

Uh oh!

Uh oh!

		if (TT.getArch() <= Triple::ArchType::UnknownArch \|\|
		TT.getArch() > Triple::ArchType::LastArchType)

[LinkerWrapper] Make -Xoffload-linker match -Xlinker semantics #101032

[LinkerWrapper] Make -Xoffload-linker match -Xlinker semantics #101032

Uh oh!

Conversation

jhuber6 commented Jul 29, 2024

Uh oh!

llvmbot commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

jhuber6 Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jplehr left a comment

Choose a reason for hiding this comment

Uh oh!

jhuber6 commented Jul 29, 2024

Uh oh!

Uh oh!

Uh oh!

[LinkerWrapper] Make `-Xoffload-linker` match `-Xlinker` semantics #101032

[LinkerWrapper] Make `-Xoffload-linker` match `-Xlinker` semantics #101032

llvmbot commented Jul 29, 2024 •

edited

Loading