[Attributor] Do not optimize away externally_initialized loads.

[Pierre-vh]

Fixes SWDEV-515029

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Pierre van Houtryve (Pierre-vh)

Changes

Fixes SWDEV-515029

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/IPO/Attributor.cpp (+2-1)
• (modified) llvm/test/Transforms/Attributor/value-simplify.ll (+19)

diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp
index a1e1a51b201b0..0865e4da1a071 100644
--- a/llvm/lib/Transforms/IPO/Attributor.cpp
+++ b/llvm/lib/Transforms/IPO/Attributor.cpp
@@ -260,7 +260,8 @@ AA::getInitialValueForObj(Attributor &A, const AbstractAttribute &QueryingAA,
       return nullptr;
   } else {
     if (!GV->hasLocalLinkage() &&
-        (GV->isInterposable() || !(GV->isConstant() && GV->hasInitializer())))
+        (GV->isInterposable() || GV->isExternallyInitialized() ||
+         !(GV->isConstant() && GV->hasInitializer())))
       return nullptr;
     if (!GV->hasInitializer())
       return UndefValue::get(&Ty);
diff --git a/llvm/test/Transforms/Attributor/value-simplify.ll b/llvm/test/Transforms/Attributor/value-simplify.ll
index a90fb54fbe74a..59160c0834980 100644
--- a/llvm/test/Transforms/Attributor/value-simplify.ll
+++ b/llvm/test/Transforms/Attributor/value-simplify.ll
@@ -12,6 +12,7 @@ declare ptr @llvm.call.preallocated.arg(token, i32)
 @ConstPtr = constant i32 0, align 4
 @ConstWeakPtr = weak constant i32 0, align 4
 @ConstWeakODRPtr = weak_odr constant i32 0, align 4
+@ExtInitZeroInit = externally_initialized constant i32 zeroinitializer, align 4
 
 ;.
 ; CHECK: @str = private unnamed_addr addrspace(4) constant [1 x i8] zeroinitializer, align 1
@@ -19,6 +20,7 @@ declare ptr @llvm.call.preallocated.arg(token, i32)
 ; CHECK: @ConstPtr = constant i32 0, align 4
 ; CHECK: @ConstWeakPtr = weak constant i32 0, align 4
 ; CHECK: @ConstWeakODRPtr = weak_odr constant i32 0, align 4
+; CHECK: @ExtInitZeroInit = externally_initialized constant i32 0, align 4
 ; CHECK: @S = external global %struct.X
 ; CHECK: @g = internal constant { [2 x ptr] } { [2 x ptr] [ptr @f1, ptr @f2] }
 ; CHECK: @x = external global i32
@@ -1651,6 +1653,23 @@ define i32 @readWeakOdrConst() {
   ret i32 %l
 }
 
+define i32 @readExtInitZeroInit() {
+; TUNIT: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
+; TUNIT-LABEL: define {{[^@]+}}@readExtInitZeroInit
+; TUNIT-SAME: () #[[ATTR2]] {
+; TUNIT-NEXT:    [[L:%.*]] = load i32, ptr @ExtInitZeroInit, align 4
+; TUNIT-NEXT:    ret i32 [[L]]
+;
+; CGSCC: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
+; CGSCC-LABEL: define {{[^@]+}}@readExtInitZeroInit
+; CGSCC-SAME: () #[[ATTR1]] {
+; CGSCC-NEXT:    [[L:%.*]] = load i32, ptr @ExtInitZeroInit, align 4
+; CGSCC-NEXT:    ret i32 [[L]]
+;
+  %l = load i32, ptr @ExtInitZeroInit
+  ret i32 %l
+}
+
 ;.
 ; TUNIT: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nosync nounwind willreturn }
 ; TUNIT: attributes #[[ATTR1]] = { memory(readwrite, argmem: none) }

[nikic]

Can that happen? I thought non-external linkage always requires an initializer.

[Pierre-vh]

You're right, I'm not sure when we should return undef in this case.
Maybe only for external GVs that don't have an initializer, and don't have externally_initialized? Not sure if that even comes up either.
@jdoerfert what should this function do?

[shiltian]

IIRC we have some global shared variables in OpenMP DeviceRTL that are marked as uninitialized via attribute.

[nikic]

It looks like the asm parser only allows globals without init if isValidDeclarationLinkage, but I don't see other calls of the function -- we're probably missing verifier enforcement for this?

[Pierre-vh]

@nikic Can I land the patch ? It'd fix a crash we've seen in the LTO pipeline.

[nikic]

On a second look, we do have a verifier check here:

llvm-project/llvm/lib/IR/Verifier.cpp

Lines 735 to 736 in e6a0ee3

	Check(!GV.isDeclaration() || GV.hasValidDeclarationLinkage(),
	"Global is external, but doesn't have external or weak linkage!", &GV);

So I believe you should just drop the !hasInitializer check here. (Or make it an assert.)