[Clang] Prevent `mlink-builtin-bitcode` from internalizing the RPC client #118661

jhuber6 · 2024-12-04T16:13:30Z

Summary:
Currently, we only use -mlink-builtin-bitcode for non-LTO NVIDIA
compiliations. This has the problem that it will internalize the RPC
client symbol which needs to be visible to the host. To counteract that,
I put retain on it, but this also prevents optimizations on the global
itself, so the passes we have that remove the symbol don't work on
OpenMP anymore. This patch does the dumbest solution, adding a special
string check for it in clang. Not the best solution, the runner up would
be to have a clang attribute for externally_initialized because those
can't be internalized, but that might have some unfortunate
side-effects. Alternatively we could make NVIDIA compilations do LTO all
the time, but that would affect some users and it's harder than I
thought.

llvmbot · 2024-12-04T16:14:04Z

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-offload

@llvm/pr-subscribers-clang

Author: Joseph Huber (jhuber6)

Changes

Summary:
Currently, we only use -mmlink-builtin-bitcode for non-LTO NVIDIA
compiliations. THis has the problem that it will internalize the RPC
client symbol which needs to be visible to the host. To counteract that,
I put retain on it, but this also prevents optimizations on the global
itself, so the passes we have that remove the symbol don't work on
OpenMP anymore. This patch does the dumbest solution, adding a special
string check for it in clang. Not the best solution, the runner up would
be to habe a clang attribute for externally_inititliazed because those
can't be internalized, but that might have some unfortunate
side-effects. Alternatively we could make NVIDIA compilations do LTO all
the time, but that would affect some users and it's harder than I
thought.

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

2 Files Affected:

(modified) clang/lib/CodeGen/CodeGenAction.cpp (+2-1)
(modified) offload/DeviceRTL/src/Misc.cpp (+1-4)

diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index cc927f44e0326e..db762b22560f28 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -246,7 +246,8 @@ bool BackendConsumer::LinkInModules(llvm::Module *M) {
           *M, std::move(LM.Module), LM.LinkFlags,
           [](llvm::Module &M, const llvm::StringSet<> &GVS) {
             internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
-              return !GV.hasName() || (GVS.count(GV.getName()) == 0);
+              return !GV.hasName() || (GVS.count(GV.getName()) == 0) ||
+                     GV.getName().starts_with("__llvm_rpc_client");
             });
           });
     } else
diff --git a/offload/DeviceRTL/src/Misc.cpp b/offload/DeviceRTL/src/Misc.cpp
index c1df477365bcb6..e6f1a341e4d769 100644
--- a/offload/DeviceRTL/src/Misc.cpp
+++ b/offload/DeviceRTL/src/Misc.cpp
@@ -113,10 +113,7 @@ void *indirectCallLookup(void *HstPtr) {
 }
 
 /// The openmp client instance used to communicate with the server.
-/// FIXME: This is marked as 'retain' so that it is not removed via
-/// `-mlink-builtin-bitcode`
-[[gnu::visibility("protected"), gnu::weak,
-  gnu::retain]] rpc::Client Client asm("__llvm_rpc_client");
+[[gnu::visibility("protected"), gnu::weak]] rpc::Client Client asm("__llvm_rpc_client");
 
 } // namespace impl
 } // namespace ompx

github-actions · 2024-12-04T16:16:58Z

✅ With the latest revision this PR passed the C/C++ code formatter.

arsenm · 2024-12-04T16:36:30Z

clang attribute for externally_initialized

I'm surprised there isn't one already. Also seems better if you're going to special case the symbol to special case it by just setting this rather than skipping internalize for it

jhuber6 · 2024-12-04T16:38:44Z

clang attribute for externally_initialized

I'm surprised there isn't one already. Also seems better if you're going to special case the symbol to special case it by just setting this rather than skipping internalize for it

I'm not sure I want it in the generic case though, because I don't know the side-effects that LLVM attribute has. This is only necessary because of this stupid mlink-builtin-bitcode thing. if there's some other way to just tell it not to internalize this global that'd be great.

jhuber6 · 2025-01-24T15:11:00Z

Ping, this is a little hacky since the real solution is to stop using -mlink-builtin-bitcode, but I'd like to let this be optimized out on NVPTX before the release.

…ient Summary: Currently, we only use `-mmlink-builtin-bitcode` for non-LTO NVIDIA compiliations. THis has the problem that it will internalize the RPC client symbol which needs to be visible to the host. To counteract that, I put `retain` on it, but this also prevents optimizations on the global itself, so the passes we have that remove the symbol don't work on OpenMP anymore. This patch does the dumbest solution, adding a special string check for it in clang. Not the best solution, the runner up would be to habe a clang attribute for `externally_inititliazed` because those can't be internalized, but that might have some unfortunate side-effects. Alternatively we could make NVIDIA compilations do LTO all the time, but that would affect some users and it's harder than I thought.

shiltian

I think the change looks fine but I'm not an expert to judge if this is the best to way to do so.

@yxsamliu @Artem-B what do you folks think?

llvm/lib/Transforms/IPO/Internalize.cpp

yxsamliu · 2025-01-27T18:55:19Z

llvm/lib/Transforms/IPO/Internalize.cpp

@@ -233,6 +233,10 @@ bool InternalizePass::internalizeModule(Module &M) {
  else
    AlwaysPreserved.insert("__stack_chk_guard");

+  // Preserve the RPC interface for GPU host callbacks when internalizing.


Like the FIXME suggested, this might be done by introducing a lambda argument when creating this pass, like MustPreserveGV, or extend MustPreserveGV to be able to preserve functions. Then when AMDGPU backend creates this pass, it may put the name of the functions that needs preserving in that lamba.

However, since there is precedence to preserve specific functions by name, I won't put a hard requirement on this PR. It is up to you.

That's what a previous version of this was, but I don't think it's correct in every case. The ideal solution would just be to remove the -mlink-builtin-bitcode option entirely.

jdoerfert

Follows the "hacky" SOTA and gets us a little closer to what we want.

jhuber6 requested review from arsenm, Artem-B, jdoerfert, JonChesterfield, shiltian and yxsamliu December 4, 2024 16:13

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. offload labels Dec 4, 2024

jhuber6 force-pushed the RetainRPC branch from 85e5c83 to 9749143 Compare December 4, 2024 16:26

jhuber6 force-pushed the RetainRPC branch from 9749143 to 3df20f0 Compare December 9, 2024 00:06

llvmbot added the llvm:transforms label Dec 9, 2024

jhuber6 force-pushed the RetainRPC branch from 7feee48 to 8ca093d Compare January 24, 2025 15:12

Merge branch 'main' into RetainRPC

5c4b1dc

shiltian reviewed Jan 27, 2025

View reviewed changes

llvm/lib/Transforms/IPO/Internalize.cpp Outdated Show resolved Hide resolved

Update llvm/lib/Transforms/IPO/Internalize.cpp

347ec18

yxsamliu reviewed Jan 27, 2025

View reviewed changes

jhuber6 requested a review from jplehr January 27, 2025 20:32

jdoerfert approved these changes Jan 28, 2025

View reviewed changes

jhuber6 merged commit 760a786 into llvm:main Jan 28, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Prevent `mlink-builtin-bitcode` from internalizing the RPC client #118661

[Clang] Prevent `mlink-builtin-bitcode` from internalizing the RPC client #118661

Uh oh!

jhuber6 commented Dec 4, 2024 •

edited by arsenm

Loading

Uh oh!

llvmbot commented Dec 4, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Dec 4, 2024 •

edited

Loading

Uh oh!

arsenm commented Dec 4, 2024

Uh oh!

jhuber6 commented Dec 4, 2024

Uh oh!

jhuber6 commented Jan 24, 2025

Uh oh!

shiltian left a comment

Uh oh!

Uh oh!

yxsamliu Jan 27, 2025

Uh oh!

jhuber6 Jan 27, 2025

Uh oh!

jdoerfert left a comment

Uh oh!

Uh oh!

Uh oh!

[Clang] Prevent mlink-builtin-bitcode from internalizing the RPC client #118661

[Clang] Prevent mlink-builtin-bitcode from internalizing the RPC client #118661

Uh oh!

Conversation

jhuber6 commented Dec 4, 2024 • edited by arsenm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

github-actions bot commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm commented Dec 4, 2024

Uh oh!

jhuber6 commented Dec 4, 2024

Uh oh!

jhuber6 commented Jan 24, 2025

Uh oh!

shiltian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yxsamliu Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

jhuber6 Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

jdoerfert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[Clang] Prevent `mlink-builtin-bitcode` from internalizing the RPC client #118661

[Clang] Prevent `mlink-builtin-bitcode` from internalizing the RPC client #118661

jhuber6 commented Dec 4, 2024 •

edited by arsenm

Loading

llvmbot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading