[HIP] Link the LLVM libc libraries in no-RDC mode

[jhuber6]

Summary:
When using the new driver we link these correctly in RDC-mode. This
patch adds logic to link them after the existing ROCm device libraries.
Because this is linked afterwards and only in RDC-mode it should not
conflict with any of the ROCm device library code.

This is not directly useful because the HIP runtime does not handle the
RPC server code, but it will still resolve missing calls to things like
memcpy, math functions, etc. If it uses an RPC call it will segfault at
runtime.

Because this is conditional on it existing I couldn't think of an easy
way to add a test.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Joseph Huber (jhuber6)

Changes

Summary:
When using the new driver we link these correctly in RDC-mode. This
patch adds logic to link them after the existing ROCm device libraries.
Because this is linked afterwards and only in RDC-mode it should not
conflict with any of the ROCm device library code.

This is not directly useful because the HIP runtime does not handle the
RPC server code, but it will still resolve missing calls to things like
memcpy, math functions, etc. If it uses an RPC call it will segfault at
runtime.

Because this is conditional on it existing I couldn't think of an easy
way to add a test.

---

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

1 Files Affected:

• (modified) clang/lib/Driver/ToolChains/HIPAMD.cpp (+19-6)

diff --git a/clang/lib/Driver/ToolChains/HIPAMD.cpp b/clang/lib/Driver/ToolChains/HIPAMD.cpp
index b4c6da0d73d13..3898a967e5b3e 100644
--- a/clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ b/clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -405,12 +405,25 @@ HIPAMDToolChain::getDeviceLibs(const llvm::opt::ArgList &DriverArgs,
     // Add instrument lib.
     auto InstLib =
         DriverArgs.getLastArgValue(options::OPT_gpu_instrument_lib_EQ);
-    if (InstLib.empty())
-      return BCLibs;
-    if (llvm::sys::fs::exists(InstLib))
-      BCLibs.emplace_back(InstLib);
-    else
-      getDriver().Diag(diag::err_drv_no_such_file) << InstLib;
+    if (!InstLib.empty()) {
+      if (llvm::sys::fs::exists(InstLib))
+        BCLibs.emplace_back(InstLib);
+      else
+        getDriver().Diag(diag::err_drv_no_such_file) << InstLib;
+    }
+  }
+
+  // Add the LLVM libc bitcode if it is available. This is added after the
+  // normal HIP libraries so it should not conflict with its definitions.
+  if (!DriverArgs.hasFlag(options::OPT_fgpu_rdc, options::OPT_fno_gpu_rdc,
+                          false)) {
+    if (std::optional<std::string> StdlibPath = getStdlibPath()) {
+      for (StringRef Library : {"libc.bc", "libm.bc"}) {
+        SmallString<128> Path(*StdlibPath);
+        llvm::sys::path::append(Path, Library);
+        BCLibs.emplace_back(Path);
+      }
+    }
   }
 
   return BCLibs;

[b-sumner]

HIP device code does not need the "LLVM libc bitcode" and I am not aware of a feature request or any other good reason to add it.

[jhuber6]

It's already linked in during the RDC mode compilation since that's shared with the other targets (new driver). However, I suppose that this does add some compilation overhead here since we need to read the bitcode file and see if it contains any needed symbols.

[b-sumner]

I would have objected to linking it in RDC mode as well if I had noticed it. What was the title of that review? And would you please remove it until/if it is actually needed?

[arsenm]

For now I think this should drive off of the llvm libc environment type

[arsenm]

Missing tests

[jhuber6]

For now I think this should drive off of the llvm libc environment type

Hm, we could definitely route that in. Right now we infer the triple as amdgcn-amd-amdhsa based off of -offload-arch so the user would need to use --offload-targets=amdgcn-amd-amdhsa-llvm --offload-arch= could work but pretty much means it's about the same as passing it manually.

[jhuber6]

So, with the new driver if you do -foffload-lto you get these libraries already since we go through the linker and pass those in as static libraries, so I guess that's a solution.

[b-sumner]

So, with the new driver if you do -foffload-lto you get these libraries already since we go through the linker and pass those in as static libraries, so I guess that's a solution.

Where did the decision to "get these libraries" come from? Linking "libc.bc" into hip is unnecessary and unwanted at this point, in my view.

[jhuber6]

Where did the decision to "get these libraries" come from? Linking "libc.bc" into hip is unnecessary and unwanted at this point, in my view.

The new driver, which hopefully be the default in HIP soonish, handles all the offloading languages more or less the same. We link these libraries by default since they're used by OpenMP and the other languages inherit that code since the implementation is totally generic. There was never a patch to turn it on in HIP specifically, it just kind of inherited it as part of the generic toolchain behavior. Static libraries only extract undefined symbols, so it should only show up if there's something that would otherwise be a linker error.

We could disable it for HIP, it's relatively trivial to pass -Xoffload-linker -lc to get the same behavior if desired manually. The non-RDC mode case is more difficult since you'd need to use the -mlink option and unfortunately you can't do -Xarch_amdgcn -Xclang -mlink-builtin-bitcode which is kind of what I was wondering if I could rectify here. But likely I'm the only one so far playing around with C / C++ standard library calls in HIP. I'm relatively close to supporting arbitrary C/C++ in HIP kernels as a part of this stuff, but if you don't think that's worthwhile to have in HIP yet I can just leave it as an OpenMP thing and give people opt-in behavior for HIP.

[b-sumner]

The new driver, which hopefully be the default in HIP soonish, handles all the offloading languages more or less the same. We link these libraries by default since they're used by OpenMP and the other languages inherit that code since the implementation is totally generic.

If this driver was really designed for all language runtimes then it should have designed in the fact that languages have different runtime libraries. HIP does not only not need the C device runtime, it doesn't need the OpenMP device runtime nor the fortran device runtime. I think languages should opt in to the libraries they need not opt out.

[arsenm]

HIP does not only not need the C device runtime,

It does. The language bakes in the assumption that all of the libc and libm calls exist in the ambient environment. A header file wrapper is not sufficient (e.g. we need workarounds like 77de8a0), and various other builtins also fail.

I think languages should opt in to the libraries they need not opt out.

There are language libraries, and system support libraries. This falls more under system support libraries that should be universally available; the question is then whether it should require explicit -lc and -lm like some toolchains do or not. I have a longer writeup about all of the issues with how we're linking everything I need to finish

[b-sumner]

HIP does not only not need the C device runtime,

It does. The language bakes in the assumption that all of the libc and libm calls exist in the ambient environment.

In host code yes, in device code it certainly does not. A hip programmer has absolutely no expectation that they can call pread() or any other of a large number libc of functions from device code. Furthermore, we have no accepted proposal for an extension to hip to support libc functions. This kind of extension requires deeper review and discussion and awareness than permitted by a LLVM pull request.

[jhuber6]

I do agree that in general we should strive to move away from -mlink-builtin-bitcode. It's realistically only necessary for this no-RDC no-LTO case. We could theoretically do code object linking if we were careful about register usage, but that opens up a can of worms. It would be nice to be able to use the LLVM libcall interface instead of just hitting backend failures for example.

Furthermore, we have no accepted proposal for an extension to hip to support libc functions.

We already provide a subset of C library calls in HIP device code, so it's mostly a question of which ones and where. Though, I suppose they're not 'true' C library calls because we rewrite them in headers to some different name.

[b-sumner]

We already provide a subset of C library calls in HIP device code, so it's mostly a question of which ones and where. Though, I suppose they're not 'true' C library calls because we rewrite them in headers to some different name.

If you mean printf, assert, and malloc, each one of those went through the process and are documented and tested. We cannot simply drop in the remainder with just a PR.