[llvm] [GlobalOpt] Handle operators separately when removing GV users (PR #84694)
Anshil Gandhi via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 24 15:01:55 PDT 2024
https://github.com/gandhi56 updated https://github.com/llvm/llvm-project/pull/84694
>From 079e6380e7f9dde2cfcfa5e2c251e90fcc78fdcc Mon Sep 17 00:00:00 2001
From: Anshil Gandhi <gandhi21299 at gmail.com>
Date: Tue, 2 Apr 2024 20:02:21 -0600
Subject: [PATCH 1/2] [GlobalOpt] Handle operators separately when removing GV
users
This patch fixes the functionality of CleanupPointerRootUsers
by separately handling cases where the global variable is
wrapped within an operator.
This patch additionally fixes some function names
to conform with the LLVM style guide.
Fixes #64680.
Change-Id: I972dc76758ced16cefcb08474b756ec89e0c047f
---
llvm/lib/Transforms/IPO/GlobalOpt.cpp | 94 ++++++++-----------
...leanup-pointer-root-users-gep-constexpr.ll | 1 -
.../Transforms/GlobalOpt/dead-store-status.ll | 2 -
llvm/test/Transforms/GlobalOpt/pr54572.ll | 8 --
4 files changed, 41 insertions(+), 64 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index b7c6d25657bc0..145d3ab146b40 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -158,12 +158,12 @@ static bool isLeakCheckerRoot(GlobalVariable *GV) {
/// 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.
-static bool IsSafeComputationToRemove(
+static bool isSafeComputationToRemove(
Value *V, function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
do {
if (isa<Constant>(V))
return true;
- if (!V->hasOneUse())
+ if (V->hasNUsesOrMore(1))
return false;
if (isa<LoadInst>(V) || isa<InvokeInst>(V) || isa<Argument>(V) ||
isa<GlobalValue>(V))
@@ -185,11 +185,21 @@ 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.
+/**
+ * Cleans up pointer root users of a global variable.
+ *
+ * This function iterates over all users of the global variable and collects
+ * all stores and memtransfer instructions. It then erases all writes and
+ * removes computation chains if they are safe to remove. Finally, it removes
+ * dead constant users of the global variable.
+ *
+ * @param GV The global variable to clean up.
+ * @param GetTLI A function reference to obtain the TargetLibraryInfo for a
+ * given function.
+ * @return True if any changes were made, false otherwise.
+ */
static bool
-CleanupPointerRootUsers(GlobalVariable *GV,
+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
@@ -202,60 +212,38 @@ CleanupPointerRootUsers(GlobalVariable *GV,
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;
-
+ // Iterate over all users of the global and collect all
+ // stores, memtransfer and memset instructions.
+ SmallVector<std::pair<Instruction *, Value *>> Writes;
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));
+ if (auto *SI = dyn_cast<StoreInst>(U)) {
+ Writes.push_back({SI, SI->getValueOperand()});
+ } else if (auto *CE = dyn_cast<ConstantExpr>(U)) {
+ if (isa<GEPOperator>(CE)) {
+ append_range(Worklist, CE->users());
}
- } 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 (auto *MTI = dyn_cast<MemTransferInst>(U)) {
+ if (MTI->getRawDest() == GV) {
+ Writes.push_back({MTI, MTI->getSource()});
}
- } 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 (auto *MSI = dyn_cast<MemSetInst>(U)) {
+ if (MSI->getRawDest() == GV) {
+ Writes.push_back({MSI, MSI->getValue()});
}
- } 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();
+ // Finally, erase all writes and remove computation chains if they are safe
+ // to remove.
+ for (auto [WriteInst, V] : Writes) {
+ if (isa<Constant>(V) || isa<Instruction>(V))
+ WriteInst->eraseFromParent();
+
+ if (auto *Inst = dyn_cast<Instruction>(V)) {
+ if (isSafeComputationToRemove(V, GetTLI))
+ RecursivelyDeleteTriviallyDeadInstructions(V);
Changed = true;
}
}
@@ -870,7 +858,7 @@ static bool OptimizeAwayTrappingUsesOfLoads(
// nor is the global.
if (AllNonStoreUsesGone) {
if (isLeakCheckerRoot(GV)) {
- Changed |= CleanupPointerRootUsers(GV, GetTLI);
+ Changed |= cleanupPointerRootUsers(GV, GetTLI);
} else {
Changed = true;
CleanupConstantGlobalUsers(GV, DL);
@@ -1496,7 +1484,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
if (isLeakCheckerRoot(GV)) {
// Delete any constant stores to the global.
- Changed = CleanupPointerRootUsers(GV, GetTLI);
+ 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.
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 26728a74d032c..66a99c15ba4d3 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,7 +99,6 @@ 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 9a8fbb8d65f0e..597c08929af90 100644
--- a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
+++ b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
@@ -4,8 +4,6 @@
; 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 962c75bd83b41..bdab4b0981d04 100644
--- a/llvm/test/Transforms/GlobalOpt/pr54572.ll
+++ b/llvm/test/Transforms/GlobalOpt/pr54572.ll
@@ -6,18 +6,10 @@
declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg)
-;.
-; 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
}
-;.
-; CHECK: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }
-;.
>From 60f5b3135e936dda81149048be0411dd53b3b060 Mon Sep 17 00:00:00 2001
From: Anshil Gandhi <Anshil.Gandhi at amd.com>
Date: Mon, 24 Jun 2024 22:01:38 +0000
Subject: [PATCH 2/2] [GlobalOpt] Merge cleanup functions into one
Change-Id: I3bada185f07d72f379ec2417257e2a07f210c261
---
llvm/lib/Transforms/IPO/GlobalOpt.cpp | 55 +++++++++++++----------
llvm/test/Transforms/GlobalOpt/pr54572.ll | 15 +++++++
2 files changed, 46 insertions(+), 24 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index 145d3ab146b40..fc56f193ad9fe 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -255,8 +255,9 @@ cleanupPointerRootUsers(GlobalVariable *GV,
/// 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) {
+static bool cleanupConstantGlobalUsers(GlobalVariable *GV,
+ const DataLayout &DL,
+ function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
Constant *Init = GV->getInitializer();
SmallVector<User *, 8> WorkList(GV->users());
SmallPtrSet<User *, 8> Visited;
@@ -306,11 +307,30 @@ static bool CleanupConstantGlobalUsers(GlobalVariable *GV,
}
}
} else if (StoreInst *SI = dyn_cast<StoreInst>(U)) {
- // 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);
+ 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);
+ }
} else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U)) {
if (II->getIntrinsicID() == Intrinsic::threadlocal_address)
append_range(WorkList, II->users());
@@ -857,12 +877,7 @@ static bool OptimizeAwayTrappingUsesOfLoads(
// If we nuked all of the loads, then none of the stores are needed either,
// nor is the global.
if (AllNonStoreUsesGone) {
- if (isLeakCheckerRoot(GV)) {
- Changed |= cleanupPointerRootUsers(GV, GetTLI);
- } else {
- Changed = true;
- CleanupConstantGlobalUsers(GV, DL);
- }
+ Changed |= cleanupConstantGlobalUsers(GV, DL, GetTLI);
if (GV->use_empty()) {
LLVM_DEBUG(dbgs() << " *** GLOBAL NOW DEAD!\n");
Changed = true;
@@ -1481,15 +1496,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
// Delete it now.
if (!GS.IsLoaded) {
LLVM_DEBUG(dbgs() << "GLOBAL NEVER LOADED: " << *GV << "\n");
-
- 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);
- }
+ Changed = cleanupConstantGlobalUsers(GV, DL, GetTLI);
// If the global is dead now, delete it.
if (GV->use_empty()) {
@@ -1513,7 +1520,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
}
// Clean up any obviously simplifiable users now.
- Changed |= CleanupConstantGlobalUsers(GV, DL);
+ Changed |= cleanupConstantGlobalUsers(GV, DL, GetTLI);
// If the global is dead now, just nuke it.
if (GV->use_empty()) {
@@ -1568,7 +1575,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
}
// Clean up any obviously simplifiable users now.
- CleanupConstantGlobalUsers(GV, DL);
+ cleanupConstantGlobalUsers(GV, DL, GetTLI);
if (GV->use_empty()) {
LLVM_DEBUG(dbgs() << " *** Substituting initializer allowed us to "
diff --git a/llvm/test/Transforms/GlobalOpt/pr54572.ll b/llvm/test/Transforms/GlobalOpt/pr54572.ll
index bdab4b0981d04..50f3ce92e104b 100644
--- a/llvm/test/Transforms/GlobalOpt/pr54572.ll
+++ b/llvm/test/Transforms/GlobalOpt/pr54572.ll
@@ -6,6 +6,9 @@
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
+;.
define void @test() {
; CHECK-LABEL: @test(
; CHECK-NEXT: ret void
@@ -13,3 +16,15 @@ define void @test() {
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) }
+;.
More information about the llvm-commits
mailing list