[llvm] [GlobalOpt] Handle operators separately when removing GV users (PR #84694)
Anshil Gandhi via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 5 00:25:35 PDT 2024
https://github.com/gandhi56 updated https://github.com/llvm/llvm-project/pull/84694
>From 09c23889c8310b7fdc3b1db7320754d354f48296 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/3] [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.
Fixes #64680.
Change-Id: I205922e4203fb032beaf2c290c0374758ea9176c
---
llvm/lib/Transforms/IPO/GlobalOpt.cpp | 85 +++++++++----------
...leanup-pointer-root-users-gep-constexpr.ll | 2 -
.../Transforms/GlobalOpt/dead-store-status.ll | 2 -
llvm/test/Transforms/GlobalOpt/pr54572.ll | 8 --
4 files changed, 40 insertions(+), 57 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index da714c9a75701..fcaf7e8c9ab2f 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -201,61 +201,56 @@ 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;
-
+ // Collect all stores to GV.
+ SmallVector<Instruction *, 8> 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));
- }
- } 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 (auto *SI = dyn_cast<StoreInst>(U)) {
+ Writes.push_back(SI);
+ } else if (auto *CE = dyn_cast<ConstantExpr>(U)) {
if (isa<GEPOperator>(CE))
append_range(Worklist, CE->users());
+ } else if (auto *MI = dyn_cast<MemIntrinsic>(U)) {
+ if (MI->getRawDest() == GV)
+ Writes.push_back(MI);
}
}
- 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);
+ auto RemoveComputationChain = [&GetTLI](Instruction *I) {
+ 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();
+ };
+
+ // Remove the store if it's value operand is a constant or a computation
+ // chain. If the value operand is a computation chain, determine if it is safe
+ // to remove it before removing the store. Remove both the store and the
+ // computation chain if it is safe to do so.
+ for (auto *Write : Writes) {
+ Value *V = Write;
+ if (auto *SI = dyn_cast<StoreInst>(V))
+ V = SI->getValueOperand();
+
+ if (isa<Constant>(V)) {
Changed = true;
+ Write->eraseFromParent();
+ } else if (isa<Instruction>(V)) {
+ if (IsSafeComputationToRemove(V, GetTLI)) {
+ Changed = true;
+ Write->eraseFromParent();
+ RemoveComputationChain(cast<Instruction>(V));
+ } else {
+ Changed = true;
+ Write->eraseFromParent();
+ }
}
}
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..db0c549ad03f4 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,14 +99,12 @@ 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
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[TMP1]], 2
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END]]
; CHECK: for.end:
-; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr [[P:%.*]], i64 8, i1 false)
; CHECK-NEXT: ret i32 0
;
entry:
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 19fe33bdd16d210b36bb231f0fb4b908880efbab Mon Sep 17 00:00:00 2001
From: Anshil Gandhi <Anshil.Gandhi at amd.com>
Date: Thu, 25 Apr 2024 17:41:36 +0000
Subject: [PATCH 2/3] Remove write operands and write instructions in two
seperate loops
Change-Id: I80e21ccffbe82dce5aff3a9b1d0d043d36baab12
---
llvm/lib/Transforms/IPO/GlobalOpt.cpp | 78 ++++++++++---------
...leanup-pointer-root-users-gep-constexpr.ll | 2 +
2 files changed, 42 insertions(+), 38 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index fcaf7e8c9ab2f..915e62a295175 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -157,7 +157,7 @@ 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))
@@ -184,11 +184,8 @@ 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,
+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
@@ -201,7 +198,21 @@ CleanupPointerRootUsers(GlobalVariable *GV,
bool Changed = false;
- // Collect all stores to GV.
+ auto RemoveComputationChain = [&GetTLI](Instruction *I) {
+ while (true) {
+ if (isAllocationFn(I, GetTLI))
+ break;
+ Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
+ I->eraseFromParent();
+ if (!J)
+ return;
+ I = J;
+ }
+ I->eraseFromParent();
+ };
+
+ // Iterate over all users of the global and collect all
+ // stores and memtransfer instructions.
SmallVector<Instruction *, 8> Writes;
SmallVector<User *> Worklist(GV->users());
while (!Worklist.empty()) {
@@ -209,48 +220,39 @@ CleanupPointerRootUsers(GlobalVariable *GV,
if (auto *SI = dyn_cast<StoreInst>(U)) {
Writes.push_back(SI);
} else if (auto *CE = dyn_cast<ConstantExpr>(U)) {
- if (isa<GEPOperator>(CE))
+ if (isa<GEPOperator>(CE)) {
append_range(Worklist, CE->users());
- } else if (auto *MI = dyn_cast<MemIntrinsic>(U)) {
- if (MI->getRawDest() == GV)
- Writes.push_back(MI);
+ }
+ } else if (auto *MTI = dyn_cast<MemTransferInst>(U)) {
+ if (MTI->getRawDest() == GV) {
+ Writes.push_back(MTI);
+ }
}
}
- auto RemoveComputationChain = [&GetTLI](Instruction *I) {
- 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();
- };
-
- // Remove the store if it's value operand is a constant or a computation
- // chain. If the value operand is a computation chain, determine if it is safe
- // to remove it before removing the store. Remove both the store and the
- // computation chain if it is safe to do so.
+ // For each write, try to remove it's operand if it is safe to do so.
+ // Finally, remove the write itself.
for (auto *Write : Writes) {
- Value *V = Write;
- if (auto *SI = dyn_cast<StoreInst>(V))
- V = SI->getValueOperand();
+ Value *V = nullptr;
+ if (isa<StoreInst>(Write)) {
+ V = cast<StoreInst>(Write)->getValueOperand();
+ } else if (isa<MemTransferInst>(Write)){
+ V = cast<MemTransferInst>(Write)->getSource();
+ } else {
+ llvm_unreachable("Unexpected instruction type");
+ }
if (isa<Constant>(V)) {
- Changed = true;
Write->eraseFromParent();
- } else if (isa<Instruction>(V)) {
- if (IsSafeComputationToRemove(V, GetTLI)) {
- Changed = true;
+ Changed = true;
+ } else if (auto *Inst = dyn_cast<Instruction>(V)) {
+ if (isSafeComputationToRemove(V, GetTLI)) {
Write->eraseFromParent();
- RemoveComputationChain(cast<Instruction>(V));
+ RemoveComputationChain(Inst);
} else {
- Changed = true;
Write->eraseFromParent();
}
+ Changed = true;
}
}
@@ -857,7 +859,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);
@@ -1483,7 +1485,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 db0c549ad03f4..420419f4caf95 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
@@ -103,8 +103,10 @@ define i32 @load_gv_from_op_remove_store(ptr %p) local_unnamed_addr {
; CHECK-NEXT: [[INC:%.*]] = add nsw i32 [[TMP1]], 1
; CHECK-NEXT: store i32 [[INC]], ptr @a, align 4
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[TMP1]], 2
+; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr @b, i8 0, i64 8, i1 false)
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END]]
; CHECK: for.end:
+; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr @b, ptr [[P:%.*]], i64 8, i1 false)
; CHECK-NEXT: ret i32 0
;
entry:
>From b969d2c20a4717dcfa0873b1acd4eba8002f3e8a Mon Sep 17 00:00:00 2001
From: Anshil Gandhi <gandhi21299 at gmail.com>
Date: Wed, 5 Jun 2024 03:22:28 -0400
Subject: [PATCH 3/3] [GlobalOpt] Collect writes in value, instruction pairs
Iterate over all users of the global variable
in question and collect each write along with
the operand that is stored into the global variable.
Remove the write instruction before attemping
to remove the operand.
---
llvm/lib/Transforms/IPO/GlobalOpt.cpp | 48 +++++++++++++--------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index 915e62a295175..87beefc67ff46 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -162,7 +162,7 @@ static bool isSafeComputationToRemove(
do {
if (isa<Constant>(V))
return true;
- if (!V->hasOneUse())
+ if (V->getNumUses() > 0)
return false;
if (isa<LoadInst>(V) || isa<InvokeInst>(V) || isa<Argument>(V) ||
isa<GlobalValue>(V))
@@ -184,6 +184,19 @@ static bool isSafeComputationToRemove(
} while (true);
}
+/**
+ * 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,
function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
@@ -213,45 +226,32 @@ cleanupPointerRootUsers(GlobalVariable *GV,
// Iterate over all users of the global and collect all
// stores and memtransfer instructions.
- SmallVector<Instruction *, 8> Writes;
+ SmallVector<std::pair<Instruction *, Value *>> Writes;
SmallVector<User *> Worklist(GV->users());
while (!Worklist.empty()) {
User *U = Worklist.pop_back_val();
if (auto *SI = dyn_cast<StoreInst>(U)) {
- Writes.push_back(SI);
+ 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 (auto *MTI = dyn_cast<MemTransferInst>(U)) {
if (MTI->getRawDest() == GV) {
- Writes.push_back(MTI);
+ Writes.push_back({MTI, MTI->getSource()});
}
}
}
- // For each write, try to remove it's operand if it is safe to do so.
- // Finally, remove the write itself.
- for (auto *Write : Writes) {
- Value *V = nullptr;
- if (isa<StoreInst>(Write)) {
- V = cast<StoreInst>(Write)->getValueOperand();
- } else if (isa<MemTransferInst>(Write)){
- V = cast<MemTransferInst>(Write)->getSource();
- } else {
- llvm_unreachable("Unexpected instruction type");
- }
+ // 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 (isa<Constant>(V)) {
- Write->eraseFromParent();
- Changed = true;
- } else if (auto *Inst = dyn_cast<Instruction>(V)) {
- if (isSafeComputationToRemove(V, GetTLI)) {
- Write->eraseFromParent();
+ if (auto *Inst = dyn_cast<Instruction>(V)) {
+ if (isSafeComputationToRemove(V, GetTLI))
RemoveComputationChain(Inst);
- } else {
- Write->eraseFromParent();
- }
Changed = true;
}
}
More information about the llvm-commits
mailing list