[llvm] Revert "[GlobalOpt] Handle operators separately when removing GV users" (PR #132971)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 25 11:19:25 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-transforms
Author: Eli Friedman (efriedma-quic)
<details>
<summary>Changes</summary>
Reverts llvm/llvm-project#<!-- -->84694 . Review was incomplete.
---
Full diff: https://github.com/llvm/llvm-project/pull/132971.diff
4 Files Affected:
- (modified) llvm/lib/Transforms/IPO/GlobalOpt.cpp (+153-32)
- (modified) llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll (+1)
- (modified) llvm/test/Transforms/GlobalOpt/dead-store-status.ll (+2)
- (modified) llvm/test/Transforms/GlobalOpt/pr54572.ll (+3-10)
``````````diff
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index 7b7b3802d7a77..2d046f09f1b2b 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -114,6 +114,55 @@ static cl::opt<int> ColdCCRelFreq(
"entry frequency, for a call site to be considered cold for enabling "
"coldcc"));
+/// Is this global variable possibly used by a leak checker as a root? If so,
+/// we might not really want to eliminate the stores to it.
+static bool isLeakCheckerRoot(GlobalVariable *GV) {
+ // A global variable is a root if it is a pointer, or could plausibly contain
+ // a pointer. There are two challenges; one is that we could have a struct
+ // the has an inner member which is a pointer. We recurse through the type to
+ // detect these (up to a point). The other is that we may actually be a union
+ // of a pointer and another type, and so our LLVM type is an integer which
+ // gets converted into a pointer, or our type is an [i8 x #] with a pointer
+ // potentially contained here.
+
+ if (GV->hasPrivateLinkage())
+ return false;
+
+ SmallVector<Type *, 4> Types;
+ Types.push_back(GV->getValueType());
+
+ unsigned Limit = 20;
+ do {
+ Type *Ty = Types.pop_back_val();
+ switch (Ty->getTypeID()) {
+ default: break;
+ case Type::PointerTyID:
+ return true;
+ case Type::FixedVectorTyID:
+ case Type::ScalableVectorTyID:
+ if (cast<VectorType>(Ty)->getElementType()->isPointerTy())
+ return true;
+ break;
+ case Type::ArrayTyID:
+ Types.push_back(cast<ArrayType>(Ty)->getElementType());
+ break;
+ case Type::StructTyID: {
+ StructType *STy = cast<StructType>(Ty);
+ if (STy->isOpaque()) return true;
+ for (Type *InnerTy : STy->elements()) {
+ if (isa<PointerType>(InnerTy)) return true;
+ if (isa<StructType>(InnerTy) || isa<ArrayType>(InnerTy) ||
+ isa<VectorType>(InnerTy))
+ Types.push_back(InnerTy);
+ }
+ break;
+ }
+ }
+ if (--Limit == 0) return true;
+ } while (!Types.empty());
+ return false;
+}
+
/// Given a value that is stored to a global but never read, determine whether
/// it's safe to remove the store and the chain of computation that feeds the
/// store.
@@ -122,7 +171,7 @@ static bool IsSafeComputationToRemove(
do {
if (isa<Constant>(V))
return true;
- if (V->hasNUsesOrMore(1))
+ if (!V->hasOneUse())
return false;
if (isa<LoadInst>(V) || isa<InvokeInst>(V) || isa<Argument>(V) ||
isa<GlobalValue>(V))
@@ -144,12 +193,90 @@ static bool IsSafeComputationToRemove(
} while (true);
}
+/// This GV is a pointer root. Loop over all users of the global and clean up
+/// any that obviously don't assign the global a value that isn't dynamically
+/// allocated.
+static bool
+CleanupPointerRootUsers(GlobalVariable *GV,
+ function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
+ // A brief explanation of leak checkers. The goal is to find bugs where
+ // pointers are forgotten, causing an accumulating growth in memory
+ // usage over time. The common strategy for leak checkers is to explicitly
+ // allow the memory pointed to by globals at exit. This is popular because it
+ // also solves another problem where the main thread of a C++ program may shut
+ // down before other threads that are still expecting to use those globals. To
+ // handle that case, we expect the program may create a singleton and never
+ // destroy it.
+
+ bool Changed = false;
+
+ // If Dead[n].first is the only use of a malloc result, we can delete its
+ // chain of computation and the store to the global in Dead[n].second.
+ SmallVector<std::pair<Instruction *, Instruction *>, 32> Dead;
+
+ SmallVector<User *> Worklist(GV->users());
+ // Constants can't be pointers to dynamically allocated memory.
+ while (!Worklist.empty()) {
+ User *U = Worklist.pop_back_val();
+ if (StoreInst *SI = dyn_cast<StoreInst>(U)) {
+ Value *V = SI->getValueOperand();
+ if (isa<Constant>(V)) {
+ Changed = true;
+ SI->eraseFromParent();
+ } else if (Instruction *I = dyn_cast<Instruction>(V)) {
+ if (I->hasOneUse())
+ Dead.push_back(std::make_pair(I, SI));
+ }
+ } else if (MemSetInst *MSI = dyn_cast<MemSetInst>(U)) {
+ if (isa<Constant>(MSI->getValue())) {
+ Changed = true;
+ MSI->eraseFromParent();
+ } else if (Instruction *I = dyn_cast<Instruction>(MSI->getValue())) {
+ if (I->hasOneUse())
+ Dead.push_back(std::make_pair(I, MSI));
+ }
+ } else if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(U)) {
+ GlobalVariable *MemSrc = dyn_cast<GlobalVariable>(MTI->getSource());
+ if (MemSrc && MemSrc->isConstant()) {
+ Changed = true;
+ MTI->eraseFromParent();
+ } else if (Instruction *I = dyn_cast<Instruction>(MTI->getSource())) {
+ if (I->hasOneUse())
+ Dead.push_back(std::make_pair(I, MTI));
+ }
+ } else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(U)) {
+ if (isa<GEPOperator>(CE))
+ append_range(Worklist, CE->users());
+ }
+ }
+
+ for (int i = 0, e = Dead.size(); i != e; ++i) {
+ if (IsSafeComputationToRemove(Dead[i].first, GetTLI)) {
+ Dead[i].second->eraseFromParent();
+ Instruction *I = Dead[i].first;
+ do {
+ if (isAllocationFn(I, GetTLI))
+ break;
+ Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
+ if (!J)
+ break;
+ I->eraseFromParent();
+ I = J;
+ } while (true);
+ I->eraseFromParent();
+ Changed = true;
+ }
+ }
+
+ GV->removeDeadConstantUsers();
+ return Changed;
+}
+
/// We just marked GV constant. Loop over all users of the global, cleaning up
/// the obvious ones. This is largely just a quick scan over the use list to
/// clean up the easy and obvious cruft. This returns true if it made a change.
-static bool CleanupConstantGlobalUsers(
- GlobalVariable *GV, const DataLayout &DL,
- function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
+static bool CleanupConstantGlobalUsers(GlobalVariable *GV,
+ const DataLayout &DL) {
Constant *Init = GV->getInitializer();
SmallVector<User *, 8> WorkList(GV->users());
SmallPtrSet<User *, 8> Visited;
@@ -199,30 +326,11 @@ static bool CleanupConstantGlobalUsers(
}
}
} else if (StoreInst *SI = dyn_cast<StoreInst>(U)) {
- auto *V = SI->getValueOperand();
- if (isa<Constant>(V)) {
- EraseFromParent(SI);
- } else if (isa<Instruction>(V)) {
- EraseFromParent(SI);
- if (IsSafeComputationToRemove(V, GetTLI))
- RecursivelyDeleteTriviallyDeadInstructions(V);
- } else if (isa<Argument>(V)) {
- if (!V->getType()->isPointerTy())
- EraseFromParent(SI);
- }
- } else if (auto *MSI = dyn_cast<MemSetInst>(U)) { // memset/cpy/mv
- if (getUnderlyingObject(MSI->getRawDest()) == GV)
- EraseFromParent(MSI);
- } else if (auto *MTI = dyn_cast<MemTransferInst>(U)) {
- auto *Src = MTI->getRawSource();
- auto *Dst = MTI->getRawDest();
- if (getUnderlyingObject(Dst) != GV)
- continue;
- if (isa<Instruction, Operator>(Src)) {
- EraseFromParent(MTI);
- if (IsSafeComputationToRemove(Src, GetTLI))
- RecursivelyDeleteTriviallyDeadInstructions(Src);
- }
+ // Store must be unreachable or storing Init into the global.
+ EraseFromParent(SI);
+ } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(U)) { // memset/cpy/mv
+ if (getUnderlyingObject(MI->getRawDest()) == GV)
+ EraseFromParent(MI);
} else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U)) {
if (II->getIntrinsicID() == Intrinsic::threadlocal_address)
append_range(WorkList, II->users());
@@ -770,7 +878,12 @@ static bool OptimizeAwayTrappingUsesOfLoads(
// If we nuked all of the loads, then none of the stores are needed either,
// nor is the global.
if (AllNonStoreUsesGone) {
- Changed |= CleanupConstantGlobalUsers(GV, DL, GetTLI);
+ if (isLeakCheckerRoot(GV)) {
+ Changed |= CleanupPointerRootUsers(GV, GetTLI);
+ } else {
+ Changed = true;
+ CleanupConstantGlobalUsers(GV, DL);
+ }
if (GV->use_empty()) {
LLVM_DEBUG(dbgs() << " *** GLOBAL NOW DEAD!\n");
Changed = true;
@@ -1384,7 +1497,15 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
// Delete it now.
if (!GS.IsLoaded) {
LLVM_DEBUG(dbgs() << "GLOBAL NEVER LOADED: " << *GV << "\n");
- Changed = CleanupConstantGlobalUsers(GV, DL, GetTLI);
+
+ if (isLeakCheckerRoot(GV)) {
+ // Delete any constant stores to the global.
+ Changed = CleanupPointerRootUsers(GV, GetTLI);
+ } else {
+ // Delete any stores we can find to the global. We may not be able to
+ // make it completely dead though.
+ Changed = CleanupConstantGlobalUsers(GV, DL);
+ }
// If the global is dead now, delete it.
if (GV->use_empty()) {
@@ -1408,7 +1529,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
}
// Clean up any obviously simplifiable users now.
- Changed |= CleanupConstantGlobalUsers(GV, DL, GetTLI);
+ Changed |= CleanupConstantGlobalUsers(GV, DL);
// If the global is dead now, just nuke it.
if (GV->use_empty()) {
@@ -1463,7 +1584,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
}
// Clean up any obviously simplifiable users now.
- CleanupConstantGlobalUsers(GV, DL, GetTLI);
+ CleanupConstantGlobalUsers(GV, DL);
if (GV->use_empty()) {
LLVM_DEBUG(dbgs() << " *** Substituting initializer allowed us to "
diff --git a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll
index 66a99c15ba4d3..26728a74d032c 100644
--- a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll
+++ b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll
@@ -99,6 +99,7 @@ define i32 @load_gv_from_op_remove_store(ptr %p) local_unnamed_addr {
; CHECK-NEXT: call void @fn0()
; CHECK-NEXT: br label [[IF_END]]
; CHECK: if.end:
+; CHECK-NEXT: store ptr [[E]], ptr getelementptr inbounds ([3 x ptr], ptr @b, i64 0, i64 2), align 16
; CHECK-NEXT: [[TMP1:%.*]] = load i32, ptr @a, align 4
; CHECK-NEXT: [[INC:%.*]] = add nsw i32 [[TMP1]], 1
; CHECK-NEXT: store i32 [[INC]], ptr @a, align 4
diff --git a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
index 597c08929af90..9a8fbb8d65f0e 100644
--- a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
+++ b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
@@ -4,6 +4,8 @@
; false. This was caught by the pass return status check that is hidden under
; EXPENSIVE_CHECKS.
+; CHECK: @global = internal unnamed_addr global ptr null, align 1
+
; CHECK-LABEL: @foo
; CHECK-NEXT: entry:
; CHECK-NEXT: ret i16 undef
diff --git a/llvm/test/Transforms/GlobalOpt/pr54572.ll b/llvm/test/Transforms/GlobalOpt/pr54572.ll
index 50f3ce92e104b..962c75bd83b41 100644
--- a/llvm/test/Transforms/GlobalOpt/pr54572.ll
+++ b/llvm/test/Transforms/GlobalOpt/pr54572.ll
@@ -7,24 +7,17 @@
declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg)
;.
-; CHECK: @b = internal unnamed_addr global ptr null
+; CHECK: @[[B:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr global ptr null
+; CHECK: @[[C:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr constant [2 x ptr] zeroinitializer
;.
define void @test() {
; CHECK-LABEL: @test(
+; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr getelementptr inbounds ([2 x ptr], ptr @c, i64 0, i64 1), i64 8, i1 false)
; CHECK-NEXT: ret void
;
call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr getelementptr inbounds ([2 x ptr], ptr @c, i64 0, i64 1), i64 8, i1 false)
ret void
}
-
-define void @neg_test(ptr %arg) {
-; CHECK-LABEL: @neg_test(
-; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr [[ARG:%.*]], i64 8, i1 false)
-; CHECK-NEXT: ret void
-;
- call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr %arg, i64 8, i1 false)
- ret void
-}
;.
; CHECK: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }
;.
``````````
</details>
https://github.com/llvm/llvm-project/pull/132971
More information about the llvm-commits
mailing list