[GlobalOpt] Don't replace aliasee with alias that has weak linkage #91483

dianqk · 2024-05-08T14:59:43Z

Don't perform the transform if the alias may be replaced at link time while someone is using the aliasee (e.g., multiple aliases potentially target it or someone calls it).

If necessary, for a conservative approach, I can backport only ab7be41 after the PR is approved.

llvmbot · 2024-05-09T14:16:35Z

@llvm/pr-subscribers-llvm-transforms

Author: DianQK (DianQK)

Changes

Fixes #91312.

Don't perform the transform if the alias may be replaced at link time while someone is using the aliasee (e.g., multiple aliases potentially target it or someone calls it).

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

4 Files Affected:

(modified) llvm/lib/Transforms/IPO/GlobalOpt.cpp (+19)
(added) llvm/test/Transforms/GlobalOpt/alias-weak.ll (+56)
(modified) llvm/test/Transforms/LowerTypeTests/cfi-nounwind-direct-call.ll (+2-2)
(modified) llvm/test/Transforms/LowerTypeTests/cfi-unwind-direct-call.ll (+4-4)

diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index c8c835115a992..53f54294c3b39 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -2215,6 +2215,17 @@ static bool hasUseOtherThanLLVMUsed(GlobalAlias &GA, const LLVMUsed &U) {
   return !U.usedCount(&GA) && !U.compilerUsedCount(&GA);
 }
 
+static bool hasMoreThanOneUseOtherThanLLVMUsed(GlobalValue &V,
+                                               const LLVMUsed &U) {
+  unsigned N = 2;
+  assert((!U.usedCount(&V) || !U.compilerUsedCount(&V)) &&
+         "We should have removed the duplicated "
+         "element from llvm.compiler.used");
+  if (U.usedCount(&V) || U.compilerUsedCount(&V))
+    ++N;
+  return V.hasNUsesOrMore(N);
+}
+
 static bool mayHaveOtherReferences(GlobalValue &GV, const LLVMUsed &U) {
   if (!GV.hasLocalLinkage())
     return true;
@@ -2224,6 +2235,7 @@ static bool mayHaveOtherReferences(GlobalValue &GV, const LLVMUsed &U) {
 
 static bool hasUsesToReplace(GlobalAlias &GA, const LLVMUsed &U,
                              bool &RenameTarget) {
+
   RenameTarget = false;
   bool Ret = false;
   if (hasUseOtherThanLLVMUsed(GA, U))
@@ -2242,6 +2254,13 @@ static bool hasUsesToReplace(GlobalAlias &GA, const LLVMUsed &U,
   //   define ... @a(...)
   Constant *Aliasee = GA.getAliasee();
   GlobalValue *Target = cast<GlobalValue>(Aliasee->stripPointerCasts());
+  // Don't perform the transform if the alias may be replaced at link time while
+  // someone is using the aliasee (e.g., multiple aliases potentially target it
+  // or someone calls it).
+  if (GA.isWeakForLinker())
+    Ret = false;
+  if (hasMoreThanOneUseOtherThanLLVMUsed(*Target, U))
+    return Ret;
   if (mayHaveOtherReferences(*Target, U))
     return Ret;
 
diff --git a/llvm/test/Transforms/GlobalOpt/alias-weak.ll b/llvm/test/Transforms/GlobalOpt/alias-weak.ll
new file mode 100644
index 0000000000000..e24085f00d714
--- /dev/null
+++ b/llvm/test/Transforms/GlobalOpt/alias-weak.ll
@@ -0,0 +1,56 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --include-generated-funcs --version 4
+; RUN: opt < %s -passes=globalopt -S | FileCheck %s
+
+@f1_alias = linkonce_odr hidden alias void (), ptr @f1
+@f2_alias = linkonce_odr hidden alias void (), ptr @f2
+
+define void @foo() {
+  call void @f1_alias()
+  ret void
+}
+
+define void @bar() {
+  call void @f1()
+  ret void
+}
+
+define void @baz() {
+  call void @f2_alias()
+  ret void
+}
+
+; We cannot use `f1_alias` to replace `f1` because they are both in use
+; and `f1_alias` could be replaced at link time.
+define internal void @f1() {
+  ret void
+}
+
+; We can use `f2_alias` to replace `f2` because `b2` is not in use.
+define internal void @f2() {
+  ret void
+}
+;.
+; CHECK: @f1_alias = linkonce_odr hidden alias void (), ptr @f1
+;.
+; CHECK-LABEL: define void @foo() local_unnamed_addr {
+; CHECK-NEXT:    call void @f1_alias()
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define void @bar() local_unnamed_addr {
+; CHECK-NEXT:    call void @f1()
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define void @baz() local_unnamed_addr {
+; CHECK-NEXT:    call void @f2_alias()
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define internal void @f1() {
+; CHECK-NEXT:    ret void
+;
+;
+; CHECK-LABEL: define linkonce_odr hidden void @f2_alias() local_unnamed_addr {
+; CHECK-NEXT:    ret void
+;
diff --git a/llvm/test/Transforms/LowerTypeTests/cfi-nounwind-direct-call.ll b/llvm/test/Transforms/LowerTypeTests/cfi-nounwind-direct-call.ll
index 2795333effd76..f05befb10658b 100644
--- a/llvm/test/Transforms/LowerTypeTests/cfi-nounwind-direct-call.ll
+++ b/llvm/test/Transforms/LowerTypeTests/cfi-nounwind-direct-call.ll
@@ -121,7 +121,7 @@ attributes #6 = { noreturn nounwind }
 ; CHECK-LABEL: define dso_local noundef range(i32 0, 2) i32 @_Z10call_catchi
 ; CHECK-SAME: (i32 noundef [[NUM:%.*]]) local_unnamed_addr #[[ATTR1:[0-9]+]] !type [[META4]] !type [[META5]] !type [[META6]] {
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    store ptr @_Z9nothrow_ei.cfi_jt, ptr @catch_ptr, align 8, !tbaa [[TBAA7:![0-9]+]]
+; CHECK-NEXT:    store ptr @.cfi.jumptable, ptr @catch_ptr, align 8, !tbaa [[TBAA7:![0-9]+]]
 ; CHECK-NEXT:    [[TOBOOL_NOT_I:%.*]] = icmp ne i32 [[NUM]], 0
 ; CHECK-NEXT:    [[DOT_I:%.*]] = zext i1 [[TOBOOL_NOT_I]] to i32
 ; CHECK-NEXT:    ret i32 [[DOT_I]]
@@ -152,7 +152,7 @@ attributes #6 = { noreturn nounwind }
 ;
 ;
 ; CHECK: Function Attrs: naked nocf_check noinline nounwind
-; CHECK-LABEL: define internal void @_Z9nothrow_ei.cfi_jt
+; CHECK-LABEL: define private void @.cfi.jumptable
 ; CHECK-SAME: () #[[ATTR5:[0-9]+]] align 8 {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    tail call void asm sideeffect "jmp ${0:c}@plt\0Aint3\0Aint3\0Aint3\0A", "s"(ptr nonnull @_Z9nothrow_ei) #[[ATTR7:[0-9]+]]
diff --git a/llvm/test/Transforms/LowerTypeTests/cfi-unwind-direct-call.ll b/llvm/test/Transforms/LowerTypeTests/cfi-unwind-direct-call.ll
index 3e1f8b97e98b8..ca540a1cc5f97 100644
--- a/llvm/test/Transforms/LowerTypeTests/cfi-unwind-direct-call.ll
+++ b/llvm/test/Transforms/LowerTypeTests/cfi-unwind-direct-call.ll
@@ -174,8 +174,8 @@ attributes #8 = { noreturn nounwind }
 ; CHECK-LABEL: define dso_local void @_Z10call_catchi(
 ; CHECK-SAME: i32 noundef [[NUM:%.*]]) local_unnamed_addr #[[ATTR0]] personality ptr @__gxx_personality_v0 !type [[META4]] !type [[META5]] !type [[META6]] {
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    store ptr @_Z7throw_ei.cfi_jt, ptr @catch_ptr, align 8, !tbaa [[TBAA11:![0-9]+]]
-; CHECK-NEXT:    invoke void @_Z7throw_ei.cfi_jt(i32 noundef [[NUM]]) #[[ATTR8:[0-9]+]]
+; CHECK-NEXT:    store ptr @.cfi.jumptable, ptr @catch_ptr, align 8, !tbaa [[TBAA11:![0-9]+]]
+; CHECK-NEXT:    invoke void @.cfi.jumptable(i32 noundef [[NUM]]) #[[ATTR8:[0-9]+]]
 ; CHECK-NEXT:            to label [[TRY_CONT:%.*]] unwind label [[LPAD:%.*]], !callees [[META13:![0-9]+]]
 ; CHECK:       lpad:
 ; CHECK-NEXT:    [[TMP0:%.*]] = landingpad { ptr, i32 }
@@ -220,7 +220,7 @@ attributes #8 = { noreturn nounwind }
 ;
 ;
 ; CHECK: Function Attrs: naked nocf_check noinline
-; CHECK-LABEL: define internal void @_Z7throw_ei.cfi_jt(
+; CHECK-LABEL: define private void @.cfi.jumptable(
 ; CHECK-SAME: ) #[[ATTR5:[0-9]+]] align 8 {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    tail call void asm sideeffect "jmp ${0:c}@plt\0Aint3\0Aint3\0Aint3\0A", "s"(ptr nonnull @_Z7throw_ei) #[[ATTR6]]
@@ -236,6 +236,6 @@ attributes #8 = { noreturn nounwind }
 ; CHECK: [[META10]] = !{!"Simple C++ TBAA"}
 ; CHECK: [[TBAA11]] = !{[[META12:![0-9]+]], [[META12]], i64 0}
 ; CHECK: [[META12]] = !{!"any pointer", [[META9]], i64 0}
-; CHECK: [[META13]] = !{ptr @_Z7throw_ei.cfi_jt}
+; CHECK: [[META13]] = !{ptr @.cfi.jumptable}
 ; CHECK: [[META14]] = !{}
 ;.

dianqk · 2024-05-09T14:17:58Z

@oskarwirga @hstk30-hw
I'm not sure if the test case updates for LowerTypeTests are correct. Could you take a look? Thanks.

avikivity · 2024-05-15T11:06:12Z

Bump. Could we get this reviewed? It's a regression from clang 16 with no workaround.

hstk30-hw · 2024-05-15T11:39:16Z

Sorry, I'm not familiar with those testcase. @nikic @efriedma-quic Can you review it?

efriedma-quic

I'm having trouble following all the different cases interleaved together. I just spent half an hour trying to follow this, and I feel like I understand less than when I started.

Please delete the Ret variable, and replace the corresponding logic with something actually comprehensible.

efriedma-quic · 2024-05-15T17:11:04Z

llvm/lib/Transforms/IPO/GlobalOpt.cpp

+  // or someone calls it).
+  if (GA.isWeakForLinker())
+    Ret = false;
+  if (hasMoreThanOneUseOtherThanLLVMUsed(*Target, U))


Since there's a stripPointerCasts() earlier, do we need to check for other uses of a cast ConstantExpr, in addition to other uses of the global itself?

Or maybe we can just drop the whole pointer-cast-stripping thing... with opaque pointer types, not sure if there's any legitimate cast that can show up here. (An addressspacecast would be allowed by the verifier, but not sure it's something we can actually generate code for.)

dianqk · 2024-05-16T04:53:51Z

I'm having trouble following all the different cases interleaved together. I just spent half an hour trying to follow this, and I feel like I understand less than when I started.

Please delete the Ret variable, and replace the corresponding logic with something actually comprehensible.

How do you feel about this PR only being submitted as ab7be41? I want to backport it to LLVM 18. I can create another PR for b901a98.

efriedma-quic · 2024-05-16T05:20:57Z

ab7be41 looks conservatively correct. Sure, just merging that to main and 18 for now seems like a plan.

This reverts commit b901a98.

dianqk · 2024-05-16T05:38:57Z

I have reverted b901a98.

efriedma-quic

LGTM

…lvm#91483) Fixes llvm#91312. Don't perform the transform if the alias may be replaced at link time. (cherry picked from commit c796900)

MaskRay · 2024-05-16T23:56:13Z

How do we get @f2 = linkonce_odr hidden alias void (), ptr @f1 in the first place? This codegen seems problematic if f2 can be defined in another module as something different.

weak/linkonce stopping the replace makes sense to me but weak_odr/linkonce_odr should not.

dianqk · 2024-05-17T00:07:39Z

How do we get @f2 = linkonce_odr hidden alias void (), ptr @f1 in the first place? This codegen seems problematic if f2 can be defined in another module as something different.

weak/linkonce stopping the replace makes sense to me but weak_odr/linkonce_odr should not.

Yes. IIUC, weak_odr/linkonce_odr should assume that the definitions of all symbols are the same.

Do you have more information? @avikivity

efriedma-quic · 2024-05-17T00:33:40Z

linkonce_odr on a function means you can replace a call to the function with the function's body (i.e. inlining). linkonce_odr on a function alias should mean the same thing, I think: a call to the alias can be replaced with the aliasee function's body. This allows a lot of useful optimizations, but it doesn't allow anything more that that. In particular, if you have an internal symbol @f1, and @f2 = linkonce_odr hidden alias void (), ptr @f1, the "odr-ness" only means that you can replace a call to @f2 with a call to @f1. It doesn't allow replacing a call to @f1 with a call to @f2, which is the transform that was causing trouble here. We sometimes refer to this as "derefinement".

Granted, the usage of linkonce_odr for aliases pointing to __tls_init stretches this definition of "equivalence" to its extreme limit; the different aliases don't do the same thing. It only works because the C++ standard explicitly allows reordering the initialization of thread-local variables.

dianqk · 2024-05-17T20:44:01Z

I don't know much about TLS, but it sounds reasonable. Though I'm not sure exactly how the IR is generated yet.

…lvm#91483) Fixes llvm#91312. Don't perform the transform if the alias may be replaced at link time. (cherry picked from commit c796900)

MaskRay · 2024-05-17T23:16:29Z

linkonce_odr on a function means you can replace a call to the function with the function's body (i.e. inlining). linkonce_odr on a function alias should mean the same thing, I think: a call to the alias can be replaced with the aliasee function's body. This allows a lot of useful optimizations, but it doesn't allow anything more that that. In particular, if you have an internal symbol @f1, and @f2 = linkonce_odr hidden alias void (), ptr @f1, the "odr-ness" only means that you can replace a call to @f2 with a call to @f1.

Agreed.

It doesn't allow replacing a call to @f1 with a call to @f2, which is the transform that was causing trouble here. We sometimes refer to this as "derefinement".

While the transformation replaces aliases with their underlying functions, it doesn't work the other way around.
This patch prevents an optimization that would replace linkonce_odr/weak_odr aliases with their aliasees.
This is my concern.

Test output before this isWeakForlinker patch. No aliasee is replaced with its alias.

define void @foo() local_unnamed_addr {
  call void @f1_alias()
  ret void
}

define void @bar() local_unnamed_addr {
  call void @f1_alias()
  ret void
}

define void @baz() local_unnamed_addr {
  call void @f2_alias()
  ret void
}

define linkonce_odr hidden void @f1_alias() local_unnamed_addr {
  ret void
}

define linkonce_odr hidden void @f2_alias() local_unnamed_addr {
  ret void
}

Test output after this isWeakForlinker patch

@f1_alias = linkonce_odr hidden alias void (), ptr @f1
@f2_alias = linkonce_odr hidden alias void (), ptr @f2

define void @foo() local_unnamed_addr {
  call void @f1_alias()
  ret void
}

define void @bar() local_unnamed_addr {
  call void @f1()
  ret void
}

define void @baz() local_unnamed_addr {
  call void @f2_alias()
  ret void
}

define internal void @f1() {
  ret void
}

define internal void @f2() {
  ret void
}

Granted, the usage of linkonce_odr for aliases pointing to __tls_init stretches this definition of "equivalence" to its extreme limit; the different aliases don't do the same thing. It only works because the C++ standard explicitly allows reordering the initialization of thread-local variables.

Agree there is some stretch about the limit.
I want to know a minimal reproduce to better understand why __tls_init was problematic.

efriedma-quic · 2024-05-17T23:56:13Z

Test output before this isWeakForlinker patch. No aliasee is replaced with its alias.

In @bar, the call to @f1 is a no-op. It's replaced with a call to @f1_alias... which non-deterministically refers to any definition of @f1_alias, which could have arbitrary effects.

While the transformation replaces aliases with their underlying functions, it doesn't work the other way around

The bug happens in the case where the underlying function has other uses. So you have an internal function with some callers, and then those callers are effectively rewritten to call a linkonce function.

The TLS case looks like the following:

struct A { A(); };
struct B { B(); };
inline thread_local A a;
static thread_local B b;
void g(A*, B*);
void f() { g(&a, &b); }
void f2() { g(&a, &b); }
void f3() { g(&a, &b); }
void f4() { g(&a, &b); }
void f5() { g(&a, &b); }

At -O0, the initialization of "a" and "b" happens in define internal void @__tls_init(). At -Oz, this becomes linkonce_odr dso_local void @_ZTH1a()... so if a is also defined in another file, we can skip the constructor for b.

MaskRay · 2024-05-18T00:49:33Z

Ah, I misread the code.
In bar, call void @f1() calling the aliasee was replaced with the alias before (RenameTarget).
This was problematic and this patch have fixed it.

This patch and its backport to release/18.x looks good to me.

I agree that some cleanup could be made to improve readability of this code.

At -O0, the initialization of "a" and "b" happens in define internal void @__tls_init(). At -Oz, this becomes linkonce_odr dso_local void @_ZTH1a()... so if a is also defined in another file, we can skip the constructor for b.

Thanks for the example.

At -O0, define internal void @__tls_init() initializes a and b.
Previously, __tls_init was incorrectly renamed (due to RenameTarget) to _ZTH1a (like derefinement).
If the linker picks the prevailing definition of _ZTH1a from another module, we would skip the constructor of b.

dianqk mentioned this pull request May 8, 2024

miscompiled TLS wrapper, likely coroutine related, with sanitizer #91312

Closed

dianqk added 2 commits May 9, 2024 21:32

Pre-commit test cases

937ab67

[GlobalOpt] Don't replace aliasee with alias that has weak linkage

ab7be41

dianqk force-pushed the globalopt-linkonce_odr branch from 8af3cc9 to 067c4aa Compare May 9, 2024 13:33

dianqk changed the title ~~[WIP][GlobalOpt] Don't replace aliasee with alias that has weak linkage~~ [GlobalOpt] Don't replace aliasee with alias that has weak linkage May 9, 2024

dianqk force-pushed the globalopt-linkonce_odr branch from 067c4aa to a778017 Compare May 9, 2024 14:13

dianqk requested review from nikic, MaskRay, oskarwirga, dtcxzyw and hstk30-hw May 9, 2024 14:15

dianqk marked this pull request as ready for review May 9, 2024 14:16

llvmbot added the llvm:transforms label May 9, 2024

[GlobalOpt] Replace aliasee if it is not in use

b901a98

dianqk force-pushed the globalopt-linkonce_odr branch from a778017 to b901a98 Compare May 9, 2024 14:18

hstk30-hw requested a review from efriedma-quic May 15, 2024 11:40

efriedma-quic reviewed May 15, 2024

View reviewed changes

Revert "[GlobalOpt] Replace aliasee if it is not in use"

b4660c7

This reverts commit b901a98.

efriedma-quic approved these changes May 16, 2024

View reviewed changes

dianqk merged commit c796900 into llvm:main May 16, 2024
3 of 4 checks passed

dianqk mentioned this pull request May 17, 2024

release/18.x: [GlobalOpt] Don't replace aliasee with alias that has weak linkage (#91483) #92468

Merged

dianqk deleted the globalopt-linkonce_odr branch May 17, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GlobalOpt] Don't replace aliasee with alias that has weak linkage #91483

[GlobalOpt] Don't replace aliasee with alias that has weak linkage #91483

dianqk commented May 8, 2024 •

edited

Loading

llvmbot commented May 9, 2024

dianqk commented May 9, 2024

avikivity commented May 15, 2024

hstk30-hw commented May 15, 2024

efriedma-quic left a comment

efriedma-quic May 15, 2024

dianqk commented May 16, 2024

efriedma-quic commented May 16, 2024

dianqk commented May 16, 2024

efriedma-quic left a comment

MaskRay commented May 16, 2024 •

edited

Loading

dianqk commented May 17, 2024

efriedma-quic commented May 17, 2024

dianqk commented May 17, 2024

MaskRay commented May 17, 2024

efriedma-quic commented May 17, 2024

MaskRay commented May 18, 2024

[GlobalOpt] Don't replace aliasee with alias that has weak linkage #91483

[GlobalOpt] Don't replace aliasee with alias that has weak linkage #91483

Conversation

dianqk commented May 8, 2024 • edited Loading

llvmbot commented May 9, 2024

dianqk commented May 9, 2024

avikivity commented May 15, 2024

hstk30-hw commented May 15, 2024

efriedma-quic left a comment

Choose a reason for hiding this comment

efriedma-quic May 15, 2024

Choose a reason for hiding this comment

dianqk commented May 16, 2024

efriedma-quic commented May 16, 2024

dianqk commented May 16, 2024

efriedma-quic left a comment

Choose a reason for hiding this comment

MaskRay commented May 16, 2024 • edited Loading

dianqk commented May 17, 2024

efriedma-quic commented May 17, 2024

dianqk commented May 17, 2024

MaskRay commented May 17, 2024

efriedma-quic commented May 17, 2024

MaskRay commented May 18, 2024

dianqk commented May 8, 2024 •

edited

Loading

MaskRay commented May 16, 2024 •

edited

Loading