Skip to content

[Attributor] Do not optimize away externally_initialized loads. #128170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 3, 2025
Merged

[Attributor] Do not optimize away externally_initialized loads. #128170

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
06c412f
[Attributor] Do not optimize away externally_initialized loads.
Pierre-vh Feb 21, 2025
9ac5ece
Comments
Pierre-vh Feb 21, 2025
4c712ce
Commentss
Pierre-vh Feb 24, 2025
88bc182
fix check
Pierre-vh Mar 3, 2025
5199b31
avoid assert only block
Pierre-vh Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions llvm/lib/Transforms/IPO/Attributor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,11 +259,16 @@ AA::getInitialValueForObj(Attributor &A, const AbstractAttribute &QueryingAA,
if (!Initializer)
return nullptr;
} else {
if (!GV->hasLocalLinkage() &&
(GV->isInterposable() || !(GV->isConstant() && GV->hasInitializer())))
return nullptr;
if (!GV->hasInitializer())
return UndefValue::get(&Ty);
if (GV->hasLocalLinkage()) {
// Uninitialized global with local linkage.
if (!GV->hasInitializer())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

return UndefValue::get(&Ty);
} else {
// Externally visible global that's either non-constant,
// or a constant with an uncertain initializer.
if (!GV->hasDefinitiveInitializer() || !GV->isConstant())
return nullptr;
}

if (!Initializer)
Initializer = GV->getInitializer();
Expand Down
19 changes: 19 additions & 0 deletions llvm/test/Transforms/Attributor/value-simplify.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ 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
; CHECK: @ConstAS3Ptr = addrspace(3) global i32 0, align 4
; 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
Expand Down Expand Up @@ -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) }
Expand Down
Loading