[llvm] [GlobalOpt] Handle operators separately when removing GV users (PR #84694)
Anshil Gandhi via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 8 06:35:19 PST 2024
https://github.com/gandhi56 updated https://github.com/llvm/llvm-project/pull/84694
>From 82f091ca939b562de3cf68f09cc10daea03d8ca3 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] Improve cleanup of pointer root users
This patch fixes the functionality of CleanupPointerRootUsers
by eliminating code redundancy and refactoring the safety checks
before removing store/memcpy/memmove value operands.
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 | 60 +++++++++----------
...leanup-pointer-root-users-gep-constexpr.ll | 1 -
.../Transforms/GlobalOpt/dead-store-status.ll | 20 +++++--
3 files changed, 42 insertions(+), 39 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index 4647c65a5c850f..b36a966e247ca8 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -160,15 +160,15 @@ 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) {
+ assert(isa<Instruction>(V) && "Expected an instruction");
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))
+ if (isa<LoadInst, InvokeInst, Argument, GlobalValue>(V))
return false;
if (isAllocationFn(V, GetTLI))
return true;
@@ -191,7 +191,7 @@ static bool IsSafeComputationToRemove(
/// 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
@@ -203,10 +203,7 @@ CleanupPointerRootUsers(GlobalVariable *GV,
// 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<Instruction *> Dead;
SmallVector<User *> Worklist(GV->users());
// Constants can't be pointers to dynamically allocated memory.
@@ -218,16 +215,16 @@ CleanupPointerRootUsers(GlobalVariable *GV,
Changed = true;
SI->eraseFromParent();
} else if (Instruction *I = dyn_cast<Instruction>(V)) {
- if (I->hasOneUse())
- Dead.push_back(std::make_pair(I, SI));
+ Dead.push_back(I);
+ SI->eraseFromParent();
}
} 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));
+ Dead.push_back(I);
+ MSI->eraseFromParent();
}
} else if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(U)) {
GlobalVariable *MemSrc = dyn_cast<GlobalVariable>(MTI->getSource());
@@ -235,8 +232,8 @@ CleanupPointerRootUsers(GlobalVariable *GV,
Changed = true;
MTI->eraseFromParent();
} else if (Instruction *I = dyn_cast<Instruction>(MTI->getSource())) {
- if (I->hasOneUse())
- Dead.push_back(std::make_pair(I, MTI));
+ Dead.push_back(I);
+ MTI->eraseFromParent();
}
} else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(U)) {
if (isa<GEPOperator>(CE))
@@ -244,20 +241,18 @@ CleanupPointerRootUsers(GlobalVariable *GV,
}
}
- 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;
+ for (auto *Inst : Dead) {
+ if (isSafeComputationToRemove(Inst, GetTLI)) {
do {
- if (isAllocationFn(I, GetTLI))
+ if (isAllocationFn(Inst, GetTLI))
break;
- Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
+ Instruction *J = dyn_cast<Instruction>(Inst->getOperand(0));
if (!J)
break;
- I->eraseFromParent();
- I = J;
+ Inst->eraseFromParent();
+ Inst = J;
} while (true);
- I->eraseFromParent();
+ Inst->eraseFromParent();
Changed = true;
}
}
@@ -269,8 +264,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;
@@ -872,10 +868,10 @@ 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);
+ cleanupConstantGlobalUsers(GV, DL, GetTLI);
}
if (GV->use_empty()) {
LLVM_DEBUG(dbgs() << " *** GLOBAL NOW DEAD!\n");
@@ -1494,11 +1490,11 @@ 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.
- Changed = CleanupConstantGlobalUsers(GV, DL);
+ Changed = cleanupConstantGlobalUsers(GV, DL, GetTLI);
}
// If the global is dead now, delete it.
@@ -1523,7 +1519,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()) {
@@ -1578,7 +1574,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/cleanup-pointer-root-users-gep-constexpr.ll b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-gep-constexpr.ll
index 26728a74d032ca..66a99c15ba4d3b 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 9a8fbb8d65f0e0..4f767253127091 100644
--- a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
+++ b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
@@ -1,19 +1,19 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt < %s -passes=globalopt -S | FileCheck %s
; When removing the store to @global in @foo, the pass would incorrectly return
; 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
-
@global = internal unnamed_addr global ptr null, align 1
; Function Attrs: nofree noinline norecurse nounwind writeonly
define i16 @foo(i16 %c) local_unnamed_addr #0 {
+; CHECK-LABEL: define i16 @foo(
+; CHECK-SAME: i16 [[C:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: ret i16 undef
+;
entry:
%local1.addr = alloca i16, align 1
store ptr %local1.addr, ptr @global, align 1
@@ -22,6 +22,14 @@ entry:
; Function Attrs: noinline nounwind writeonly
define i16 @bar() local_unnamed_addr #1 {
+; CHECK-LABEL: define i16 @bar(
+; CHECK-SAME: ) local_unnamed_addr #[[ATTR1:[0-9]+]] {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[LOCAL2:%.*]] = alloca [1 x i16], align 1
+; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 1, ptr nonnull [[LOCAL2]])
+; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 1, ptr nonnull [[LOCAL2]])
+; CHECK-NEXT: ret i16 undef
+;
entry:
%local2 = alloca [1 x i16], align 1
call void @llvm.lifetime.start.p0(i64 1, ptr nonnull %local2)
More information about the llvm-commits
mailing list