[MergeFuncs] Don't introduce calls to (linkonce,weak)_odr functions. #125050

fhahn · 2025-01-30T11:45:36Z

Avoid creating new calls to linkonce_odr/weak_odr functions when
merging 2 functions.

Consider 2 functions below, both present in 2 modules. Without this
patch, MergeFuncs in the first module may optimize A to call B while in
another module B may call A via a function pointer, i.e. no merging is
happening.

Note that the 2 optimizations are vaild in isolation, but the linker then
could pick A from module 1 (which calls B) and B from module 2 which
calls A, introducing an infinte call cycle.

There may be other linkage types we need to be more careful about as
well.

define weak_odr hidden void @"A"(ptr %p) {
entry:
  tail call void @"foo"(ptr %p)
  ret void
}

define weak_odr hidden void @"B"(ptr %p) {
entry:
  tail call void @"foo"(ptr %p)
  ret void
}

llvmbot · 2025-01-30T11:46:09Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Avoid creating new calls to weak_odr functions when merging 2 functions.

Consider 2 functions below, both present in 2 modules. Without this
patch, MergeFuncs in the first module may optimize A to call B and in
the second module B to call A.

Note that the 2 optimizations are vaild in isolation, but the linker then
could pick A from module 1 (which calls B) and B from module 2 which
calls A, introducing an infinte call cycle.

There may be other linkage types we need to be more careful about as
well.

define weak_odr hidden void @"A"(ptr %p) {
entry:
  tail call void @"foo"(ptr %p)
  ret void
}

define weak_odr hidden void @"B"(ptr %p) {
entry:
  tail call void @"foo"(ptr %p)
  ret void
}

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

2 Files Affected:

(modified) llvm/lib/Transforms/IPO/MergeFunctions.cpp (+2-2)
(added) llvm/test/Transforms/MergeFunc/merge-weak-odr.ll (+112)

diff --git a/llvm/lib/Transforms/IPO/MergeFunctions.cpp b/llvm/lib/Transforms/IPO/MergeFunctions.cpp
index e8508416f54275..d991ec64445ee9 100644
--- a/llvm/lib/Transforms/IPO/MergeFunctions.cpp
+++ b/llvm/lib/Transforms/IPO/MergeFunctions.cpp
@@ -891,8 +891,8 @@ bool MergeFunctions::writeThunkOrAlias(Function *F, Function *G) {
 
 // Merge two equivalent functions. Upon completion, Function G is deleted.
 void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
-  if (F->isInterposable()) {
-    assert(G->isInterposable());
+  if (F->isInterposable() || G->hasWeakODRLinkage()) {
+    assert(G->isInterposable() || G->hasWeakODRLinkage());
 
     // Both writeThunkOrAlias() calls below must succeed, either because we can
     // create aliases for G and NewF, or because a thunk for F is profitable.
diff --git a/llvm/test/Transforms/MergeFunc/merge-weak-odr.ll b/llvm/test/Transforms/MergeFunc/merge-weak-odr.ll
new file mode 100644
index 00000000000000..d2e89092b9a826
--- /dev/null
+++ b/llvm/test/Transforms/MergeFunc/merge-weak-odr.ll
@@ -0,0 +1,112 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --include-generated-funcs --version 5
+; RUN: opt -p mergefunc -S %s | FileCheck %s
+
+define weak_odr hidden void @weak_odr_caller_of_foo_1(ptr %p) {
+entry:
+  tail call void @foo(ptr %p)
+  tail call void @foo(ptr %p)
+  tail call void @foo(ptr %p)
+  ret void
+}
+
+define weak_odr hidden void @weak_odr_caller_of_foo_2(ptr %p) {
+entry:
+  tail call void @foo(ptr %p)
+  tail call void @foo(ptr %p)
+  tail call void @foo(ptr %p)
+  ret void
+}
+
+declare void @foo(ptr)
+
+define hidden void @weak_odr_caller_of_bar_1(ptr %p) {
+entry:
+  tail call void @bar(ptr %p)
+  tail call void @bar(ptr %p)
+  tail call void @bar(ptr %p)
+  ret void
+}
+
+define weak_odr hidden void @non_weak_caller_of_bar_2(ptr %p) {
+entry:
+  tail call void @bar(ptr %p)
+  tail call void @bar(ptr %p)
+  tail call void @bar(ptr %p)
+  ret void
+}
+
+declare void @bar(ptr)
+
+define hidden void @non_weak_caller_of_zar_1(ptr %p) {
+entry:
+  tail call void @zar(ptr %p)
+  tail call void @zar(ptr %p)
+  tail call void @zar(ptr %p)
+  ret void
+}
+
+define weak_odr hidden void @weak_odr_caller_of_zar_2(ptr %p) {
+entry:
+  tail call void @zar(ptr %p)
+  tail call void @zar(ptr %p)
+  tail call void @zar(ptr %p)
+  ret void
+}
+
+declare void @zar(ptr)
+; CHECK-LABEL: define private void @0(
+; CHECK-SAME: ptr [[P:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    tail call void @foo(ptr [[P]])
+; CHECK-NEXT:    tail call void @foo(ptr [[P]])
+; CHECK-NEXT:    tail call void @foo(ptr [[P]])
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define weak_odr hidden void @non_weak_caller_of_bar_2(
+; CHECK-SAME: ptr [[P:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    tail call void @bar(ptr [[P]])
+; CHECK-NEXT:    tail call void @bar(ptr [[P]])
+; CHECK-NEXT:    tail call void @bar(ptr [[P]])
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define private void @1(
+; CHECK-SAME: ptr [[P:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    tail call void @zar(ptr [[P]])
+; CHECK-NEXT:    tail call void @zar(ptr [[P]])
+; CHECK-NEXT:    tail call void @zar(ptr [[P]])
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define weak_odr hidden void @weak_odr_caller_of_foo_2(
+; CHECK-SAME: ptr [[TMP0:%.*]]) {
+; CHECK-NEXT:    tail call void @[[GLOB0:[0-9]+]](ptr [[TMP0]])
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define weak_odr hidden void @weak_odr_caller_of_foo_1(
+; CHECK-SAME: ptr [[TMP0:%.*]]) {
+; CHECK-NEXT:    tail call void @[[GLOB0]](ptr [[TMP0]])
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define hidden void @weak_odr_caller_of_bar_1(
+; CHECK-SAME: ptr [[TMP0:%.*]]) {
+; CHECK-NEXT:    tail call void @non_weak_caller_of_bar_2(ptr [[TMP0]])
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define weak_odr hidden void @weak_odr_caller_of_zar_2(
+; CHECK-SAME: ptr [[TMP0:%.*]]) {
+; CHECK-NEXT:    tail call void @[[GLOB1:[0-9]+]](ptr [[TMP0]])
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define hidden void @non_weak_caller_of_zar_1(
+; CHECK-SAME: ptr [[TMP0:%.*]]) {
+; CHECK-NEXT:    tail call void @[[GLOB1]](ptr [[TMP0]])
+; CHECK-NEXT:    ret void
+;

github-actions · 2025-01-30T16:09:55Z

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

kyulee-com · 2025-01-30T17:51:40Z

llvm/test/Transforms/MergeFunc/merge-linkonce-odr.ll

+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define hidden void @non_linkonce_caller_of_zar_1(


*caller_of_bar* and *caller_of_zar* have identical structures except for their order in the source, but their generated code differs. Can we ensure deterministic code generation regardless of their order?

Yes that's why I added the test case, but forgot to flag it here

fhahn · 2025-01-31T16:52:34Z

The scenario below should hopefully better illustrate what is causing the issue. It would be great to confirm if I am not missing anything that makes the scenario invalid.

Module X

define linkonce_odr void @"A"() {
  call void @"foo"()
}

define linkonce_odr void @"B"() {
  call void @"foo"()
}

Module Y

global @"g" = @"B"

define linkonce_odr void @"A"() {
  %l = load @"g"
  call void %l()
}

define linkonce_odr void @"B"() {
  call void @"foo"()
}

@"A" and @"B" in both modules should be semantically equivalent

Module X after function merging:

define linkonce_odr void @"A"() {
  call void @"foo"()
}

define linkonce_odr void @"B"() {
  call void @"A"()
}

Module Y is unchanged.

Then the linker picks @"A" from module Y and @"B" from module X. Now there's an infinite call cycle

teresajohnson · 2025-01-31T17:11:15Z

llvm/lib/Transforms/IPO/MergeFunctions.cpp

@@ -891,8 +891,10 @@ bool MergeFunctions::writeThunkOrAlias(Function *F, Function *G) {

 // Merge two equivalent functions. Upon completion, Function G is deleted.
 void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
-  if (F->isInterposable()) {
-    assert(G->isInterposable());
+  if (F->isInterposable() || G->hasWeakODRLinkage() ||


I'm not familiar with this code - what is this guarding? And is it intended that you are checking F in the first check but G in the newly added checks?

Do we need to worry about AvailableExternallyLinkage? ThinLTO converts non-prevailing linkonce_odr and those won't stick around as out of line copies, so might be ok as I think the infinite loop you are describing can't happen in that case?

If I read correctly, this is going to happen with or without LTO, and having the prevailing symbol info doesn't help here. Creating the infinite loop is one thing, but having one call the other (if either of them is interposable) will break the runtime since providing a runtime replacement will cause either no replacement or replacing one function will cause replace both functions. In this case, you can only use the thunk to merge them.

So I think we just just check here:

if (F->isInterposable() || G->isInterposable())

The check here is to decide whether we cannot use F instead of G. If the condition is true, we don't use F for G but introduce new function with the implementation of F that is called by both F and G.

If the condition is false, we update all callers of G to use F and update G to call F.

As @cachemeifyoucan the issue occurs without (Thin)LTO.

Unfortunately linkonce_odr & weak_odr aren't considered interposable (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/GlobalValue.h#L426). linkonce_odr and weak_odr functions are guaranteed to be replaced by something semantically equivalent, but they could have different semantically equivalent implementations in modules, where merging can be done in one module but not the other.

I tried to provide a more detailed example here: #125050 (comment)

I see, it is rely on ODR to assume if the replacement happens, it is functionally equivalent so it is safe.

Maybe it is safer to replace all references of isInterposable() in this pass with the stricter definition here that includes ODR.

Unfortunately replacing all reference of isInterposable gives worse results, in particular, if we have a function F that is not interposable and not linkonce/weak ODR and another function G that is link once/weak ODR, then G should be able to call F.

I also updated the comparator to order ODR functions before non-ODR functions to consistently support the case above.

Tests for #125050.

…tions. Tests for llvm/llvm-project#125050.

fhahn

I found another case where we would introduce calls to linkonce_odr/weak_odr functions, if we merge 3+ linkonce_odr/weak_odr functions.

To fix this, I updated the order to always ensure that the ODR functions come first.

I also updated the code to replace all direct callers of ODR functions to call the thunk

Avoid creating new calls to weak_odr functions when merging 2 functions. Consider 2 functions below, both present in 2 modules. Without this patch, MergeFuncs in the first module may optimize A to call B and in the second module B to call A. Note that the 2 optimizations are vaild in isolation, but the linker then could pick A from module 1 (which calls B) and B from module 2 which calls A, introducing an infinte call cycle. There may be other linkage types we need to be more careful about as well. define weak_odr hidden void @"A"(ptr %p) { entry: tail call void @"foo"(ptr %p) ret void } define weak_odr hidden void @"B"(ptr %p) { entry: tail call void @"foo"(ptr %p) ret void }

…functions. (#125050) Avoid creating new calls to linkonce_odr/weak_odr functions when merging 2 functions, as this may introduce an infinite call cycle. Consider 2 functions below, both present in 2 modules. Module X -- define linkonce_odr void @"A"() { call void @"foo"() } define linkonce_odr void @"B"() { call void @"foo"() } --- Module Y --- global @"g" = @"B" define linkonce_odr void @"A"() { %l = load @"g" call void %l() } define linkonce_odr void @"B"() { call void @"foo"() } --- @"A" and @"B" in both modules are semantically equivalent Module X after function merging: --- define linkonce_odr void @"A"() { call void @"foo"() } define linkonce_odr void @"B"() { call void @"A"() } --- Module Y is unchanged. Then the linker picks @"A" from module Y and @"B" from module X. Now there's an infinite call cycle PR: llvm/llvm-project#125050

Tests for llvm#125050. (cherry picked from commit aa77085)

…lvm#125050) Avoid creating new calls to linkonce_odr/weak_odr functions when merging 2 functions, as this may introduce an infinite call cycle. Consider 2 functions below, both present in 2 modules. Module X -- define linkonce_odr void @"A"() { call void @"foo"() } define linkonce_odr void @"B"() { call void @"foo"() } --- Module Y --- global @"g" = @"B" define linkonce_odr void @"A"() { %l = load @"g" call void %l() } define linkonce_odr void @"B"() { call void @"foo"() } --- @"A" and @"B" in both modules are semantically equivalent Module X after function merging: --- define linkonce_odr void @"A"() { call void @"foo"() } define linkonce_odr void @"B"() { call void @"A"() } --- Module Y is unchanged. Then the linker picks @"A" from module Y and @"B" from module X. Now there's an infinite call cycle PR: llvm#125050 (cherry picked from commit f10e0f7)

fhahn requested review from aschwaighofer, cachemeifyoucan, boomanaiden154 and kyulee-com January 30, 2025 11:45

llvmbot added the llvm:transforms label Jan 30, 2025

fhahn force-pushed the mergefunc-weak-odr branch from 43ff025 to ad40253 Compare January 30, 2025 16:06

fhahn changed the title ~~[MergeFuncs] Don't introduce calls to weak_odr functions.~~ [MergeFuncs] Don't introduce calls to {linkonce,weak}_odr functions. Jan 30, 2025

kyulee-com reviewed Jan 30, 2025

View reviewed changes

cachemeifyoucan requested a review from teresajohnson January 30, 2025 18:04

teresajohnson reviewed Jan 31, 2025

View reviewed changes

fhahn force-pushed the mergefunc-weak-odr branch from cbe754c to 293f3ec Compare February 3, 2025 18:37

cachemeifyoucan approved these changes Feb 13, 2025

View reviewed changes

fhahn added a commit that referenced this pull request Feb 24, 2025

[MergeFunc] Add tests for (merging weak|linkonce)_odr functions.

aa77085

Tests for #125050.

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Feb 24, 2025

Automerge: [MergeFunc] Add tests for (merging weak|linkonce)_odr func…

3258c67

…tions. Tests for llvm/llvm-project#125050.

fhahn force-pushed the mergefunc-weak-odr branch from 293f3ec to dad2026 Compare February 24, 2025 15:54

fhahn changed the title ~~[MergeFuncs] Don't introduce calls to {linkonce,weak}_odr functions.~~ [MergeFuncs] Don't introduce calls to (linkonce,weak)_odr functions. Feb 24, 2025

fhahn commented Feb 24, 2025

View reviewed changes

fhahn force-pushed the mergefunc-weak-odr branch from dad2026 to b1ce602 Compare February 24, 2025 15:57

fhahn added 6 commits February 25, 2025 12:03

!fixup also include linkonce_odr

e5c826f

!fixup fix formatting

d19f98d

!fixup Document and sort by weak/linkince ODR.

8e006cf

!fixup

02bd494

!fixup fix formatting

c077b08

fhahn force-pushed the mergefunc-weak-odr branch from b1ce602 to c077b08 Compare February 25, 2025 12:04

!fixup update clang test

354817f

llvmbot added the clang Clang issues not falling into any other category label Feb 25, 2025

fhahn merged commit f10e0f7 into llvm:main Feb 25, 2025
12 checks passed

fhahn deleted the mergefunc-weak-odr branch February 25, 2025 15:55

fhahn mentioned this pull request Feb 25, 2025

[MergeFuncs] Don't introduce calls to (linkonce,weak)_odr functions. swiftlang/llvm-project#10112

Merged

fhahn added a commit to fhahn/llvm-project that referenced this pull request Mar 6, 2025

[MergeFunc] Add tests for (merging weak|linkonce)_odr functions.

3be8df7

Tests for llvm#125050. (cherry picked from commit aa77085)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[MergeFuncs] Don't introduce calls to (linkonce,weak)_odr functions. #125050

[MergeFuncs] Don't introduce calls to (linkonce,weak)_odr functions. #125050

Uh oh!

fhahn commented Jan 30, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jan 30, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jan 30, 2025 •

edited

Loading

Uh oh!

kyulee-com Jan 30, 2025

Uh oh!

fhahn Jan 30, 2025

Uh oh!

fhahn commented Jan 31, 2025

Uh oh!

teresajohnson Jan 31, 2025

Uh oh!

cachemeifyoucan Jan 31, 2025

Uh oh!

fhahn Jan 31, 2025

Uh oh!

cachemeifyoucan Jan 31, 2025

Uh oh!

fhahn Feb 3, 2025

Uh oh!

fhahn left a comment

Uh oh!

Uh oh!

Uh oh!

[MergeFuncs] Don't introduce calls to (linkonce,weak)_odr functions. #125050

[MergeFuncs] Don't introduce calls to (linkonce,weak)_odr functions. #125050

Uh oh!

Conversation

fhahn commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

kyulee-com Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

fhahn Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

fhahn commented Jan 31, 2025

Uh oh!

teresajohnson Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

fhahn Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

fhahn Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fhahn commented Jan 30, 2025 •

edited

Loading

llvmbot commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Jan 30, 2025 •

edited

Loading