[GlobalOpt] Don't replace the aliasee if it has other references.

[DianQK]

Summary

As long as aliasee has @llvm.used or @llvm.compiler.used references, we cannot do the related replace or delete operations. Even if it is a Local Linkage, we cannot infer if there is no other use for it, such as asm or other future added cases.

This fixes https://github.com/rust-lang/rust/issues/108030.

This is a minimal reproduction that works on macOS and Linux (x86_64).

linking-error.zip1 KBDownload

[nikic]

Looks reasonable to me, but @MaskRay or @pcc should review this wrt alias semantics. Note that you have a failing GlobalOpt test.

llvm/lib/Transforms/IPO/GlobalOpt.cpp

2254

The "OtherThanLLVMUsed" part here no longer makes sense?

[DianQK]

Fix test.

[DianQK]

llvm/lib/Transforms/IPO/GlobalOpt.cpp

2254

Yes, thanks.

[MaskRay]

How does rustc use llvm.compiler.used or llvm.uses?

As long as aliasee has @llvm.used or @llvm.compiler.used references, we cannot do the related replace or delete operations. Even if it is a Local Linkage, we cannot infer if there is no other use for it, such as asm or other future added cases.

This is vague. I think we need tests to demonstrate that it is unsafe to replace. If you can write an inline asm expression or module-level asm to demonstrate the issue, change the test to reflect the case.

Do we have a use case with an internal alias and an aliasee which are not in llvm.used/llvm.compiler.used? If no, we should add such a test.

[pcc]

To be honest, I would rather we look at removing OptimizeGlobalAliases altogether. As I explained in D65118, these kinds of trivial alias "optimizations" on their own can be more harmful than helpful.

[DianQK]

In D145293#4176631, @MaskRay wrote:

How does rustc use llvm.compiler.used or llvm.uses?

I have found at least two cases where rustc is used.

The first is the sym keyword used to keep the referenced symbol in the global_asm macro, which in this case would be marked as llvm.compiler.used.

extern "Rust" {
    pub fn foo1();
    pub fn foo2();
}

fn bar1() {}
fn bar2() {}

core::arch::global_asm!(".global foo1
foo1: jmp {}
.global foo2
foo2: jmp {}
", sym bar1, sym bar2);

The second is to used declare that the symbol is reserved.

#[used]
fn foo() {}

If we focus only on the first case, https://rust.godbolt.org/z/Ph419hqWv is a minimal example.
Due to the LTO pipeline of rustc, this optimized IR will be optimized again, which triggers the current problem, where bar2 replaces bar1 and removes bar1.

If you can write an inline asm expression or module-level asm to demonstrate the issue, change the test to reflect the case.

Added.

Do we have a use case with an internal alias and an aliasee which are not in llvm.used/llvm.compiler.used? If no, we should add such a test.

I think the @fa2 in the previous llvm/test/Transforms/GlobalOpt/alias-used.ll use case corresponds to this use case.

It looks like a similar problem occurs in MergeFunctions, see Rust #99059 and D127751.

[DianQK]

In D145293#4176933, @pcc wrote:

To be honest, I would rather we look at removing OptimizeGlobalAliases altogether. As I explained in D65118, these kinds of trivial alias "optimizations" on their own can be more harmful than helpful.

Due to my limited skills, I can't evaluate the benefits of OptimizeGlobalAliases.
As you said, I find optimizing this kind of problem a bit complicated, especially when it comes to different linkage handling.
How about adding some additional assertions when removing aliases? If possible, this one that at least problems like could be detected much earlier.

[DianQK]

Add comments

[DianQK]

ping?

[DianQK]

Fixes CI.

[DianQK]

polly/test/Support/dumpfunction.ll

23

@Meinersbur GlobalOpt may optimize alias, and this patch modifies the behavior. In dumpfunction.ll, this alias doesn't seem to make sense. Can I remove it?

[DianQK]

Rebase and ping. At least this prevents an unsound case.

[nikic]

LGTM

[DianQK]

Thanks. Rebase and ensure that the pre-merge check passes.

llvm/lib/Transforms/IPO/GlobalOpt.cpp

if (!GA.hasOneUse())
     // Strictly more than one use. So at least one is not in llvm.used and
     // llvm.compiler.used.
     return true;
 
   // Exactly one use. Check if it is in llvm.used or llvm.compiler.used.
   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(GlobalAlias &GA, const LLVMUsed &U) {
-  if (!GA.hasLocalLinkage())
+static bool mayHaveOtherReferences(GlobalValue &GV, const LLVMUsed &U) {
+  if (!GV.hasLocalLinkage())
     return true;
 
-  return U.usedCount(&GA) || U.compilerUsedCount(&GA);
+  return U.usedCount(&GV) || U.compilerUsedCount(&GV);
 }
 
 static bool hasUsesToReplace(GlobalAlias &GA, const LLVMUsed &U,
                              bool &RenameTarget) {
   RenameTarget = false;
   bool Ret = false;
   if (hasUseOtherThanLLVMUsed(GA, U))
     Ret = true;
 
   // If the alias is externally visible, we may still be able to simplify it.
   if (!mayHaveOtherReferences(GA, U))
     return Ret;
 
-  // If the aliasee has internal linkage, give it the name and linkage
-  // of the alias, and delete the alias.  This turns:
+  // If the aliasee has internal linkage and no other references (e.g.,
+  // @llvm.used, @llvm.compiler.used), give it the name and linkage of the
+  // alias, and delete the alias. This turns:
   //   define internal ... @f(...)
   //   @a = alias ... @f
   // into:
   //   define ... @a(...)
   Constant *Aliasee = GA.getAliasee();
   GlobalValue *Target = cast<GlobalValue>(Aliasee->stripPointerCasts());
-  if (!Target->hasLocalLinkage())
-    return Ret;
-
-  // Do not perform the transform if multiple aliases potentially target the
-  // aliasee. This check also ensures that it is safe to replace the section
-  // and other attributes of the aliasee with those of the alias.
-  if (hasMoreThanOneUseOtherThanLLVMUsed(*Target, U))
+  if (mayHaveOtherReferences(*Target, U))
     return Ret;
 
   RenameTarget = true;
   return true;
 }
 
 static bool
 OptimizeGlobalAliases(Module &M,

llvm/test/Transforms/GlobalOpt/X86/alias-used-with-asm.ll

+; RUN: opt < %s -passes=globalopt -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+module asm ".global foo1"
+module asm "foo1: jmp bar1"
+module asm ".global foo2"
+module asm "foo2: jmp bar2"
+
+; The `llvm.compiler.used` indicates that `foo1` and `foo2` have associated symbol references in asm.
+; Checking globalopt does not remove these two symbols.
[e-mail] = appending global [2 x ptr] [ptr @bar1, ptr @bar2], section "llvm.metadata"
+; CHECK: @llvm.compiler.used = appending global [2 x ptr] [ptr @bar1, ptr @bar2], section "llvm.metadata"
+
+@bar2 = internal alias void (), ptr @bar1
+; CHECK: @bar2 = internal alias void (), ptr @bar1
+
+define internal void @bar1() {
+; CHECK: define internal void @bar1()
+  ret void
+}

llvm/test/Transforms/GlobalOpt/alias-used-address-space.ll

 ; RUN: opt -S -passes=globalopt < %s | FileCheck %s
 
 target datalayout = "p:32:32:32-p1:16:16:16"
 
 @c = hidden addrspace(1) global i8 42
 
 @i = internal addrspace(1) global i8 42
-
-; CHECK: @ia = internal addrspace(1) global i8 42
-@ia = internal alias i8, ptr addrspace(1) @i
+; CHECK: @i = internal addrspace(1) global i8 42
 
 @llvm.used = appending global [1 x ptr] [ptr addrspacecast (ptr addrspace(1) @ca to ptr)], section "llvm.metadata"
 ; CHECK-DAG: @llvm.used = appending global [1 x ptr] [ptr addrspacecast (ptr addrspace(1) @ca to ptr)], section "llvm.metadata"
 
 @llvm.compiler.used = appending global [2 x ptr] [ptr addrspacecast(ptr addrspace(1) @ia to ptr), ptr addrspacecast (ptr addrspace(1) @i to ptr)], section "llvm.metadata"
-; CHECK-DAG: @llvm.compiler.used = appending global [1 x ptr] [ptr addrspacecast (ptr addrspace(1) @ia to ptr)], section "llvm.metadata"
+; CHECK-DAG: @llvm.compiler.used = appending global [2 x ptr] [ptr addrspacecast (ptr addrspace(1) @i to ptr), ptr addrspacecast (ptr addrspace(1) @ia to ptr)], section "llvm.metadata"
 
 @sameAsUsed = global [1 x ptr] [ptr addrspacecast(ptr addrspace(1) @ca to ptr)]
 ; CHECK-DAG: @sameAsUsed = local_unnamed_addr global [1 x ptr] [ptr addrspacecast (ptr addrspace(1) @c to ptr)]
 
+@ia = internal alias i8, ptr addrspace(1) @i
+; CHECK: @ia = internal alias i8, ptr addrspace(1) @i
+
 @ca = internal alias i8, ptr addrspace(1) @c
 ; CHECK: @ca = internal alias i8, ptr addrspace(1) @c
 
 define ptr addrspace(1) @h() {
   ret ptr addrspace(1) @ca
 }

llvm/test/Transforms/GlobalOpt/alias-used.ll

 ; RUN: opt < %s -passes=globalopt -S | FileCheck %s
 
 @c = dso_local global i8 42
 
+; CHECK: @i = internal global i8 42
 @i = internal global i8 42
-; CHECK: @ia = internal global i8 42
+; CHECK-DAG: @ia = internal alias i8, ptr @i
 @ia = internal alias i8, ptr @i
 
 @llvm.used = appending global [3 x ptr] [ptr @fa, ptr @f, ptr @ca], section "llvm.metadata"
 ; CHECK-DAG: @llvm.used = appending global [3 x ptr] [ptr @ca, ptr @f, ptr @fa], section "llvm.metadata"
 
 @llvm.compiler.used = appending global [4 x ptr] [ptr @fa3, ptr @fa, ptr @ia, ptr @i], section "llvm.metadata"
-; CHECK-DAG: @llvm.compiler.used = appending global [2 x ptr] [ptr @fa3, ptr @ia], section "llvm.metadata"
+; CHECK-DAG: @llvm.compiler.used = appending global [3 x ptr] [ptr @fa3, ptr @i, ptr @ia], section "llvm.metadata"
 
 @sameAsUsed = global [3 x ptr] [ptr @fa, ptr @f, ptr @ca]
 ; CHECK-DAG: @sameAsUsed = local_unnamed_addr global [3 x ptr] [ptr @f, ptr @f, ptr @c]
 
 @other = global ptr @fa
 ; CHECK-DAG: @other = local_unnamed_addr global ptr @f
 
 @fa = internal alias void (), ptr @f

polly/test/Support/dumpfunction.ll

 ; void caller(int n, double A[]) {
 ;   for (int i = 0; i < n; i += 1)
 ;     callee(n, A, i);
 ; }
 
 
 %unrelated_type = type { i32 }
 
-@callee_alias = dso_local unnamed_addr alias void(i32, ptr, i32), ptr @callee
-
 define internal void @callee(i32 %n, ptr noalias nonnull %A, i32 %i) #0 {
 entry:
   br label %for
 
 for:
   %j = phi i32 [0, %entry], [%j.inc, %inc]
   %j.cmp = icmp slt i32 %j, %n
   br i1 %j.cmp, label %body, label %exit