[MergeFunc] Remove discardables function before writing alias or thunk. #128865

fhahn · 2025-02-26T11:38:35Z

Update writeThunkOrAlias to only create an alias or thunk if it is actually needed. Drop discardable linkone_odr functions if they are not used before.

llvmbot · 2025-02-26T11:39:16Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Update writeThunkOrAlias to only create an alias or thunk if it is actually needed. Drop discardable linkone_odr functions if they are not used before.

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

4 Files Affected:

(modified) llvm/lib/Transforms/IPO/MergeFunctions.cpp (+22-14)
(modified) llvm/test/Transforms/MergeFunc/linkonce.ll (+1-7)
(modified) llvm/test/Transforms/MergeFunc/linkonce_odr.ll (+1-7)
(modified) llvm/test/Transforms/MergeFunc/merge-linkonce-odr.ll (+1-13)

diff --git a/llvm/lib/Transforms/IPO/MergeFunctions.cpp b/llvm/lib/Transforms/IPO/MergeFunctions.cpp
index 9cc159830b17d..013dd0e04457b 100644
--- a/llvm/lib/Transforms/IPO/MergeFunctions.cpp
+++ b/llvm/lib/Transforms/IPO/MergeFunctions.cpp
@@ -283,9 +283,10 @@ class MergeFunctions {
   // Replace G with an alias to F (deleting function G)
   void writeAlias(Function *F, Function *G);
 
-  // Replace G with an alias to F if possible, or a thunk to F if possible.
-  // Returns false if neither is the case.
-  bool writeThunkOrAlias(Function *F, Function *G);
+  // If needed, replace G with an alias to F if possible, or a thunk to F if
+  // profitable. Returns false if neither is the case. If \p G is not needed
+  // (e.g. it is discardable and linkonce_odr), \p G is removed directly.
+  bool writeThunkOrAliasIfNeeded(Function *F, Function *G);
 
   /// Replace function F with function G in the function tree.
   void replaceFunctionInTree(const FunctionNode &FN, Function *G);
@@ -875,9 +876,15 @@ void MergeFunctions::writeAlias(Function *F, Function *G) {
   ++NumAliasesWritten;
 }
 
-// Replace G with an alias to F if possible, or a thunk to F if
-// profitable. Returns false if neither is the case.
-bool MergeFunctions::writeThunkOrAlias(Function *F, Function *G) {
+// If needed, replace G with an alias to F if possible, or a thunk to F if
+// profitable. Returns false if neither is the case. If \p G is not needed (e.g.
+// it is discardable and linkonce_odr), \p G is removed directly.
+bool MergeFunctions::writeThunkOrAliasIfNeeded(Function *F, Function *G) {
+  if (G->isDiscardableIfUnused() &&
+      G->use_empty() && !MergeFunctionsPDI) {
+    G->eraseFromParent();
+    return true;
+  }
   if (canCreateAliasFor(G)) {
     writeAlias(F, G);
     return true;
@@ -904,9 +911,10 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
     assert((!isODR(G) || isODR(F)) &&
            "if G is ODR, F must also be ODR due to ordering");
 
-    // Both writeThunkOrAlias() calls below must succeed, either because we can
-    // create aliases for G and NewF, or because a thunk for F is profitable.
-    // F here has the same signature as NewF below, so that's what we check.
+    // Both writeThunkOrAliasIfNeeded() calls below must succeed, either because
+    // we can create aliases for G and NewF, or because a thunk for F is
+    // profitable. F here has the same signature as NewF below, so that's what
+    // we check.
     if (!canCreateThunkFor(F) &&
         (!canCreateAliasFor(F) || !canCreateAliasFor(G)))
       return;
@@ -930,13 +938,13 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
     if (isODR(F))
       replaceDirectCallers(NewF, F);
 
-    // We collect alignment before writeThunkOrAlias that overwrites NewF and
-    // G's content.
+    // We collect alignment before writeThunkOrAliasIfNeeded that overwrites
+    // NewF and G's content.
     const MaybeAlign NewFAlign = NewF->getAlign();
     const MaybeAlign GAlign = G->getAlign();
 
-    writeThunkOrAlias(F, G);
-    writeThunkOrAlias(F, NewF);
+    writeThunkOrAliasIfNeeded(F, G);
+    writeThunkOrAliasIfNeeded(F, NewF);
 
     if (NewFAlign || GAlign)
       F->setAlignment(std::max(NewFAlign.valueOrOne(), GAlign.valueOrOne()));
@@ -975,7 +983,7 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
       return;
     }
 
-    if (writeThunkOrAlias(F, G)) {
+    if (writeThunkOrAliasIfNeeded(F, G)) {
       ++NumFunctionsMerged;
     }
   }
diff --git a/llvm/test/Transforms/MergeFunc/linkonce.ll b/llvm/test/Transforms/MergeFunc/linkonce.ll
index 2f648edc62408..2b65234b02cc2 100644
--- a/llvm/test/Transforms/MergeFunc/linkonce.ll
+++ b/llvm/test/Transforms/MergeFunc/linkonce.ll
@@ -43,14 +43,8 @@ define linkonce i32 @funA(i32 %x, i32 %y) {
 ; CHECK-NEXT:    ret i32 [[SUM3]]
 ;
 ;
-; CHECK-LABEL: define linkonce i32 @funC(
-; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
-; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0:[0-9]+]](i32 [[TMP0]], i32 [[TMP1]])
-; CHECK-NEXT:    ret i32 [[TMP3]]
-;
-;
 ; CHECK-LABEL: define linkonce i32 @funB(
 ; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
-; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0]](i32 [[TMP0]], i32 [[TMP1]])
+; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0:[0-9]+]](i32 [[TMP0]], i32 [[TMP1]])
 ; CHECK-NEXT:    ret i32 [[TMP3]]
 ;
diff --git a/llvm/test/Transforms/MergeFunc/linkonce_odr.ll b/llvm/test/Transforms/MergeFunc/linkonce_odr.ll
index ecbe6f08ab8c2..4ff34c7cd17fe 100644
--- a/llvm/test/Transforms/MergeFunc/linkonce_odr.ll
+++ b/llvm/test/Transforms/MergeFunc/linkonce_odr.ll
@@ -43,14 +43,8 @@ define linkonce_odr i32 @funA(i32 %x, i32 %y) {
 ; CHECK-NEXT:    ret i32 [[SUM3]]
 ;
 ;
-; CHECK-LABEL: define linkonce_odr i32 @funC(
-; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
-; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0:[0-9]+]](i32 [[TMP0]], i32 [[TMP1]])
-; CHECK-NEXT:    ret i32 [[TMP3]]
-;
-;
 ; CHECK-LABEL: define linkonce_odr i32 @funB(
 ; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
-; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0]](i32 [[TMP0]], i32 [[TMP1]])
+; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0:[0-9]+]](i32 [[TMP0]], i32 [[TMP1]])
 ; CHECK-NEXT:    ret i32 [[TMP3]]
 ;
diff --git a/llvm/test/Transforms/MergeFunc/merge-linkonce-odr.ll b/llvm/test/Transforms/MergeFunc/merge-linkonce-odr.ll
index 7e815e1f50d64..1a622bfae72c4 100644
--- a/llvm/test/Transforms/MergeFunc/merge-linkonce-odr.ll
+++ b/llvm/test/Transforms/MergeFunc/merge-linkonce-odr.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --include-generated-funcs --version 5
-; RUN: opt -p mergefunc -S %s | FileCheck %s
+; RUN: opt -p mergefunc -S %s | FileCheck --implicit-check-not=linkonce_odr_caller_of_foo_2 --implicit-check-not=linkonce_odr_caller_of_foo_1 %s
 
 define void @caller_of_callers(ptr %p) {
   call void @linkonce_odr_caller_of_foo_1(ptr %p)
@@ -125,15 +125,3 @@ declare void @zar(ptr)
 ; CHECK-NEXT:    tail call void @zar(ptr [[P]])
 ; CHECK-NEXT:    ret void
 ;
-;
-; CHECK-LABEL: define linkonce_odr hidden void @linkonce_odr_caller_of_foo_2(
-; CHECK-SAME: ptr [[TMP0:%.*]]) {
-; CHECK-NEXT:    tail call void @[[GLOB0]](ptr [[TMP0]])
-; CHECK-NEXT:    ret void
-;
-;
-; CHECK-LABEL: define linkonce_odr hidden void @linkonce_odr_caller_of_foo_1(
-; CHECK-SAME: ptr [[TMP0:%.*]]) {
-; CHECK-NEXT:    tail call void @[[GLOB0]](ptr [[TMP0]])
-; CHECK-NEXT:    ret void
-;

github-actions · 2025-02-26T11:42:11Z

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

teresajohnson · 2025-02-26T15:47:27Z

llvm/test/Transforms/MergeFunc/linkonce.ll

 ; CHECK-LABEL: define linkonce i32 @funB(
 ; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
-; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0]](i32 [[TMP0]], i32 [[TMP1]])
+; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0:[0-9]+]](i32 [[TMP0]], i32 [[TMP1]])


Why is GLOB0 even defined as it isn't used anywhere now? Is this meant to be calling the new @0 defined above? If so, that's presumably where GLOB0 should be defined. Ditto for the linkonce_odr.ll test below

I was going to suggest that you add a check that funC no longer exists, but I see that even before this patch the test contains -implicit-check-not=funC - how was that working since there was a funC before?

Also, what happens to funA? Is it also deleted as a result of this patch? Should that be checked as well?

Yep, the thunk that we introduce may also end up dead and could be cleaned up. I can also look into that separately?

I was going to suggest that you add a check that funC no longer exists, but I see that even before this patch the test contains -implicit-check-not=funC - how was that working since there was a funC before?

I think the reason this worked before is that the check lines in the test were specifically checking for funC, disabling the implicit check not.

Added implicit-check-not for funA

Yep, the thunk that we introduce may also end up dead and could be cleaned up. I can also look into that separately?

Is this in reference to GLOB0? I'm confused then - what does GLOB0 end up equaling if not the introduced @0?

Yes it should be @0 from the defined function above (define private i32 @0(). Looks like the update scripts don't use patterns at the definition, but use it here. I could update the checks manually to check for @0 here?

Oh I see, that is automatically generated. Hmm, seems like a bug in the update script, especially since there is currently no use of the captured GLOB0 value! I'll leave it up to you then, manually fixing it might be reasonable but then would get messed up if the update script gets run again at some point.

teresajohnson · 2025-02-26T15:55:05Z

llvm/lib/Transforms/IPO/MergeFunctions.cpp

+// profitable. Returns false if neither is the case. If \p G is not needed (e.g.
+// it is discardable and linkonce_odr), \p G is removed directly.
+bool MergeFunctions::writeThunkOrAliasIfNeeded(Function *F, Function *G) {
+  if (G->isDiscardableIfUnused() && G->use_empty() && !MergeFunctionsPDI) {


Is G's lack of uses created by this pass? Why not let a subsequent DCE invocation remove it? Also, if MergeFunctionsPDI is enabled, but G is discardable and unused, wouldn't DCE still go ahead and remove?

It is done by the pass, when merging a function we may update all callers of one functions to call another, leaving around some unused functions.

GlobalDCE would pick those up, but MergeFunctions is run after GlobalDCE, presumably to avoid spending time on trying to merge dead functions.

MergeFunc already removes some functions if they become dead.

teresajohnson · 2025-02-26T15:55:37Z

llvm/lib/Transforms/IPO/MergeFunctions.cpp

-bool MergeFunctions::writeThunkOrAlias(Function *F, Function *G) {
+// If needed, replace G with an alias to F if possible, or a thunk to F if
+// profitable. Returns false if neither is the case. If \p G is not needed (e.g.
+// it is discardable and linkonce_odr), \p G is removed directly.


the "and linkonce_odr" doesn't seem correct, as other linkage types are also removed. Did you mean "and unused"? Ditto for comment in header.

linkonce_odr was meant as an example, but I changed it to i.e. instead of e.g. and update to discardable and unused, thanks

teresajohnson

lgtm

teresajohnson · 2025-02-27T03:35:02Z

llvm/test/Transforms/MergeFunc/linkonce.ll

 ; CHECK-LABEL: define linkonce i32 @funB(
 ; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
-; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0]](i32 [[TMP0]], i32 [[TMP1]])
+; CHECK-NEXT:    [[TMP3:%.*]] = tail call i32 @[[GLOB0:[0-9]+]](i32 [[TMP0]], i32 [[TMP1]])


Oh I see, that is automatically generated. Hmm, seems like a bug in the update script, especially since there is currently no use of the captured GLOB0 value! I'll leave it up to you then, manually fixing it might be reasonable but then would get messed up if the update script gets run again at some point.

Update writeThunkOrAlias to only create an alias or thunk if it is actually needed. Drop discardable linkone_odr functions if they are not used before.

fhahn

I updated the tests manually for now, will see if I can put up a patch for the update script separately, thanks!

…ias or thunk. (#128865) Update writeThunkOrAlias to only create an alias or thunk if it is actually needed. Drop discardable linkone_odr functions if they are not used before. PR: llvm/llvm-project#128865

llvm-ci · 2025-02-27T15:50:22Z

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-4 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/15376

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM :: ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll' FAILED ********************
Exit Code: 2

Command Output (stderr):
--
RUN: at line 1: /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli -jit-kind=orc-lazy -compile-threads=2 -thread-entry hello /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll | /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
+ /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
+ /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli -jit-kind=orc-lazy -compile-threads=2 -thread-entry hello /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
 #0 0x00000001013d35fc llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100ebf5fc)
 #1 0x00000001013d1680 llvm::sys::RunSignalHandlers() (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100ebd680)
 #2 0x00000001013d3cb8 SignalHandler(int, __siginfo*, void*) (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100ebfcb8)
 #3 0x0000000197916584 (/usr/lib/system/libsystem_platform.dylib+0x18047a584)
 #4 0x00000001978e521c (/usr/lib/system/libsystem_pthread.dylib+0x18044921c)
 #5 0x000000019780bad0 (/usr/lib/libc++.1.dylib+0x18036fad0)
 #6 0x0000000100f7bdb4 void llvm::detail::UniqueFunctionBase<void, llvm::Expected<llvm::DenseMap<llvm::orc::SymbolStringPtr, llvm::orc::ExecutorSymbolDef, llvm::DenseMapInfo<llvm::orc::SymbolStringPtr, void>, llvm::detail::DenseMapPair<llvm::orc::SymbolStringPtr, llvm::orc::ExecutorSymbolDef>>>>::CallImpl<llvm::orc::Platform::lookupInitSymbols(llvm::orc::ExecutionSession&, llvm::DenseMap<llvm::orc::JITDylib*, llvm::orc::SymbolLookupSet, llvm::DenseMapInfo<llvm::orc::JITDylib*, void>, llvm::detail::DenseMapPair<llvm::orc::JITDylib*, llvm::orc::SymbolLookupSet>> const&)::$_45>(void*, llvm::Expected<llvm::DenseMap<llvm::orc::SymbolStringPtr, llvm::orc::ExecutorSymbolDef, llvm::DenseMapInfo<llvm::orc::SymbolStringPtr, void>, llvm::detail::DenseMapPair<llvm::orc::SymbolStringPtr, llvm::orc::ExecutorSymbolDef>>>&) (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100a67db4)
 #7 0x0000000100f77ba4 llvm::orc::AsynchronousSymbolQuery::handleComplete(llvm::orc::ExecutionSession&)::RunQueryCompleteTask::run() (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100a63ba4)
 #8 0x00000001010345c8 void* std::__1::__thread_proxy[abi:un170006]<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, llvm::orc::DynamicThreadPoolTaskDispatcher::dispatch(std::__1::unique_ptr<llvm::orc::Task, std::__1::default_delete<llvm::orc::Task>>)::$_0>>(void*) (/Users/buildbot/buildbot-root/aarch64-darwin/build/bin/lli+0x100b205c8)
 #9 0x00000001978e5f94 (/usr/lib/system/libsystem_pthread.dylib+0x180449f94)
#10 0x00000001978e0d34 (/usr/lib/system/libsystem_pthread.dylib+0x180444d34)
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll

--

********************

…k. (llvm#128865) Update writeThunkOrAlias to only create an alias or thunk if it is actually needed. Drop discardable linkone_odr functions if they are not used before. PR: llvm#128865

fhahn requested review from gchatelet, aschwaighofer, cachemeifyoucan, teresajohnson and kyulee-com February 26, 2025 11:38

llvmbot added the llvm:transforms label Feb 26, 2025

teresajohnson reviewed Feb 26, 2025

View reviewed changes

fhahn force-pushed the mergefunc-remove branch from 92b3c79 to 67b75a1 Compare February 26, 2025 21:29

teresajohnson approved these changes Feb 27, 2025

View reviewed changes

fhahn added 4 commits February 27, 2025 12:25

[MergeFunc] Remove discardables function before writing alias or thunk.

1548cd4

Update writeThunkOrAlias to only create an alias or thunk if it is actually needed. Drop discardable linkone_odr functions if they are not used before.

!fixup fix formatting

9697fbf

!fixup address latest comments, thanks

7e0d0eb

!fixup Don't use pattern to check unnamed function in call.

fb08c27

fhahn force-pushed the mergefunc-remove branch from 67b75a1 to fb08c27 Compare February 27, 2025 12:55

fhahn commented Feb 27, 2025

View reviewed changes

fhahn merged commit 3afc3f4 into llvm:main Feb 27, 2025
11 checks passed

fhahn deleted the mergefunc-remove branch February 27, 2025 15:27

huaatian mentioned this pull request Feb 28, 2025

fix live interval empty issue huaatian/llvm-project#1

Open

[MergeFunc] Remove discardables function before writing alias or thunk. #128865

[MergeFunc] Remove discardables function before writing alias or thunk. #128865

Uh oh!

Conversation

fhahn commented Feb 26, 2025

Uh oh!

llvmbot commented Feb 26, 2025

Uh oh!

github-actions bot commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Feb 27, 2025

Uh oh!

Uh oh!

github-actions bot commented Feb 26, 2025 •

edited

Loading