[llvm] [GlobalOpt] Handle operators separately when removing GV users (PR #84694)
Anshil Gandhi via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 12 10:56:15 PDT 2024
https://github.com/gandhi56 updated https://github.com/llvm/llvm-project/pull/84694
>From 800a13d830310cea2ccd93322e82ae55f63f8267 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] [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.
---
llvm/lib/Transforms/IPO/GlobalOpt.cpp | 105 +++++++++---------
...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, 53 insertions(+), 63 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index b7c6d25657bc0..5987767d70356 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->getNumUses() > 0)
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,51 @@ 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;
+ auto RemoveComputationChain = [&GetTLI](Instruction *I) {
+ do {
+ if (isAllocationFn(I, GetTLI))
+ break;
+ Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
+ I->eraseFromParent();
+ if (!J)
+ return;
+ I = J;
+ } while (true);
+ I->eraseFromParent();
+ };
+ // 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))
+ RemoveComputationChain(Inst);
Changed = true;
}
}
@@ -870,7 +871,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 +1497,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) }
-;.
More information about the llvm-commits
mailing list