Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang][NVPTX] Allow passing arguments to the linker while standalone #73030

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Nov 21, 2023

Summary:
We support standalone compilation for the NVPTX architecture using
'nvlink' as our linker. Because of the special handling required to
transform input files to cubins, as nvlink expects for some reason, we
didn't use the standard AddLinkerInput method. However, this also meant
that we weren't forwarding options passed with -Wl to the linker. Add
this support in for the standalone toolchain path.

Revived from https://reviews.llvm.org/D149978

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 21, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 21, 2023

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Joseph Huber (jhuber6)

Changes

Summary:
We support standalone compilation for the NVPTX architecture using
'nvlink' as our linker. Because of the special handling required to
transform input files to cubins, as nvlink expects for some reason, we
didn't use the standard AddLinkerInput method. However, this also meant
that we weren't forwarding options passed with -Wl to the linker. Add
this support in for the standalone toolchain path.

Revived from https://reviews.llvm.org/D149978


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

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Cuda.cpp (+21-22)
  • (modified) clang/test/Driver/cuda-cross-compiling.c (+8)
diff --git a/clang/lib/Driver/ToolChains/Cuda.cpp b/clang/lib/Driver/ToolChains/Cuda.cpp
index e95ff98e6c940f1..5ef8b4455c23f13 100644
--- a/clang/lib/Driver/ToolChains/Cuda.cpp
+++ b/clang/lib/Driver/ToolChains/Cuda.cpp
@@ -611,35 +611,34 @@ void NVPTX::Linker::ConstructJob(Compilation &C, const JobAction &JA,
       continue;
     }
 
-    // Currently, we only pass the input files to the linker, we do not pass
-    // any libraries that may be valid only for the host.
-    if (!II.isFilename())
-      continue;
-
     // The 'nvlink' application performs RDC-mode linking when given a '.o'
     // file and device linking when given a '.cubin' file. We always want to
     // perform device linking, so just rename any '.o' files.
     // FIXME: This should hopefully be removed if NVIDIA updates their tooling.
-    auto InputFile = getToolChain().getInputFilename(II);
-    if (llvm::sys::path::extension(InputFile) != ".cubin") {
-      // If there are no actions above this one then this is direct input and we
-      // can copy it. Otherwise the input is internal so a `.cubin` file should
-      // exist.
-      if (II.getAction() && II.getAction()->getInputs().size() == 0) {
-        const char *CubinF =
-            Args.MakeArgString(getToolChain().getDriver().GetTemporaryPath(
-                llvm::sys::path::stem(InputFile), "cubin"));
-        if (llvm::sys::fs::copy_file(InputFile, C.addTempFile(CubinF)))
-          continue;
+    if (II.isFilename()) {
+      auto InputFile = getToolChain().getInputFilename(II);
+      if (llvm::sys::path::extension(InputFile) != ".cubin") {
+        // If there are no actions above this one then this is direct input and
+        // we can copy it. Otherwise the input is internal so a `.cubin` file
+        // should exist.
+        if (II.getAction() && II.getAction()->getInputs().size() == 0) {
+          const char *CubinF =
+              Args.MakeArgString(getToolChain().getDriver().GetTemporaryPath(
+                  llvm::sys::path::stem(InputFile), "cubin"));
+          if (llvm::sys::fs::copy_file(InputFile, C.addTempFile(CubinF)))
+            continue;
 
-        CmdArgs.push_back(CubinF);
+          CmdArgs.push_back(CubinF);
+        } else {
+          SmallString<256> Filename(InputFile);
+          llvm::sys::path::replace_extension(Filename, "cubin");
+          CmdArgs.push_back(Args.MakeArgString(Filename));
+        }
       } else {
-        SmallString<256> Filename(InputFile);
-        llvm::sys::path::replace_extension(Filename, "cubin");
-        CmdArgs.push_back(Args.MakeArgString(Filename));
+        CmdArgs.push_back(Args.MakeArgString(InputFile));
       }
-    } else {
-      CmdArgs.push_back(Args.MakeArgString(InputFile));
+    } else if (!II.isNothing()) {
+      II.getInputArg().renderAsInput(Args, CmdArgs);
     }
   }
 
diff --git a/clang/test/Driver/cuda-cross-compiling.c b/clang/test/Driver/cuda-cross-compiling.c
index 12d0af3b45f32f6..5a52496838813ee 100644
--- a/clang/test/Driver/cuda-cross-compiling.c
+++ b/clang/test/Driver/cuda-cross-compiling.c
@@ -77,3 +77,11 @@
 // RUN:   | FileCheck -check-prefix=LOWERING %s
 
 // LOWERING: -cc1" "-triple" "nvptx64-nvidia-cuda" {{.*}} "-mllvm" "--nvptx-lower-global-ctor-dtor"
+
+//
+// Test passing arguments directly to nvlink.
+//
+// RUN: %clang -target nvptx64-nvidia-cuda -Wl,-v -Wl,a,b -### %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=LINKER-ARGS %s
+
+// LINKER-ARGS: nvlink{{.*}}"-v"{{.*}}"a" "b"

@jhuber6
Copy link
Contributor Author

jhuber6 commented Nov 21, 2023

I have a review up to change the issue I was observing in CMake when building the libc project #73030. That is required for this to work when compiling the test suite.

Summary:
We support standalone compilation for the NVPTX architecture using
'nvlink' as our linker. Because of the special handling required to
transform input files to cubins, as nvlink expects for some reason, we
didn't use the standard AddLinkerInput method. However, this also meant
that we weren't forwarding options passed with -Wl to the linker. Add
this support in for the standalone toolchain path.

Revived from https://reviews.llvm.org/D149978
jhuber6 added a commit to jhuber6/llvm-project that referenced this pull request Nov 23, 2023
Summary:
There are a few default options that LLVM adds that can be problematic
for runtimes builds. These options are generally intended to handle
building LLVM itself, but are also added when building in a runtimes
mode. One such issue I've run into is that in `libc` we deliberately use
`--target` to use a different device toolchain, which doesn't support
some linker arguments passed via `-Wl`. This is observed in
llvm#73030 when attempting to use
these options.
jhuber6 added a commit that referenced this pull request Nov 27, 2023
Summary:
There are a few default options that LLVM adds that can be problematic
for runtimes builds. These options are generally intended to handle
building LLVM itself, but are also added when building in a runtimes
mode. One such issue I've run into is that in `libc` we deliberately use
`--target` to use a different device toolchain, which doesn't support
some linker arguments passed via `-Wl`. This is observed in
#73030 when attempting to use
these options.

This patch completely removes these default arguments.

The consensus is that any issues created by this patch should ultimately
be solved on a per-runtime basis.
jhuber6 added a commit that referenced this pull request Nov 27, 2023
Summary:
There are a few default options that LLVM adds that can be problematic
for runtimes builds. These options are generally intended to handle
building LLVM itself, but are also added when building in a runtimes
mode. One such issue I've run into is that in `libc` we deliberately use
`--target` to use a different device toolchain, which doesn't support
some linker arguments passed via `-Wl`. This is observed in
#73030 when attempting to use
these options.

This patch completely removes these default arguments.

The consensus is that any issues created by this patch should ultimately
be solved on a per-runtime basis.
zmodem added a commit that referenced this pull request Nov 28, 2023
This appears to have caused a variety of breakages, see comments on the PR.

> Summary:
> There are a few default options that LLVM adds that can be problematic
> for runtimes builds. These options are generally intended to handle
> building LLVM itself, but are also added when building in a runtimes
> mode. One such issue I've run into is that in `libc` we deliberately use
> `--target` to use a different device toolchain, which doesn't support
> some linker arguments passed via `-Wl`. This is observed in
> #73030 when attempting to use
> these options.
>
> This patch completely removes these default arguments.
>
> The consensus is that any issues created by this patch should ultimately
> be solved on a per-runtime basis.

This reverts commit ee922e6.
@jhuber6
Copy link
Contributor Author

jhuber6 commented Nov 29, 2023

Ping

@jhuber6
Copy link
Contributor Author

jhuber6 commented Feb 19, 2024

Ping, once #81921 lands this patch won't cause any issues with the libc build like it does currently so I'd like to land this afterwards.

@jhuber6 jhuber6 merged commit d5a15f3 into llvm:main Feb 22, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants