[llvm] [GVN] Improve processBlock for instruction erasure (PR #131753)
Madhur Amilkanthwar via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 29 00:58:49 PDT 2025
https://github.com/madhur13490 updated https://github.com/llvm/llvm-project/pull/131753
>From a54e0a5f1b281a5999ad0e68eea9a94012e74a01 Mon Sep 17 00:00:00 2001
From: Madhur Amilkanthwar <madhura at nvidia.com>
Date: Tue, 4 Mar 2025 00:43:40 -0800
Subject: [PATCH 1/3] [GVN] Improve processBlock for instruction erasure
This patch deletes the instructions right away by using
the approapriate iterators. Thus avoids collecting the instructions
in a vector and then doing the erasure.
---
llvm/include/llvm/Transforms/Scalar/GVN.h | 9 ++--
llvm/lib/Transforms/Scalar/GVN.cpp | 62 +++++------------------
2 files changed, 20 insertions(+), 51 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h
index ffdf57cd3d8f8..7aa43a467382e 100644
--- a/llvm/include/llvm/Transforms/Scalar/GVN.h
+++ b/llvm/include/llvm/Transforms/Scalar/GVN.h
@@ -25,6 +25,8 @@
#include "llvm/IR/ValueHandle.h"
#include "llvm/Support/Allocator.h"
#include "llvm/Support/Compiler.h"
+#include "llvm/Transforms/Utils/AssumeBundleBuilder.h"
+#include "llvm/Transforms/Utils/Local.h"
#include <cstdint>
#include <optional>
#include <utility>
@@ -137,9 +139,11 @@ class GVNPass : public PassInfoMixin<GVNPass> {
/// This removes the specified instruction from
/// our various maps and marks it for deletion.
- void markInstructionForDeletion(Instruction *I) {
+ void doInstructionDeletion(Instruction *I) {
+ salvageKnowledge(I, AC);
+ salvageDebugInfo(*I);
VN.erase(I);
- InstrsToErase.push_back(I);
+ removeInstruction(I);
}
DominatorTree &getDominatorTree() const { return *DT; }
@@ -306,7 +310,6 @@ class GVNPass : public PassInfoMixin<GVNPass> {
// propagate to any successors. Entries added mid-block are applied
// to the remaining instructions in the block.
SmallMapVector<Value *, Value *, 4> ReplaceOperandsWithMap;
- SmallVector<Instruction *, 8> InstrsToErase;
// Map the block to reversed postorder traversal number. It is used to
// find back edge easily.
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index c0ae944ee82f2..0ea9c3c2f7e75 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -69,7 +69,6 @@
#include "llvm/Support/Compiler.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/raw_ostream.h"
-#include "llvm/Transforms/Utils/AssumeBundleBuilder.h"
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
#include "llvm/Transforms/Utils/Local.h"
#include "llvm/Transforms/Utils/SSAUpdater.h"
@@ -728,8 +727,9 @@ void GVNPass::ValueTable::erase(Value *V) {
/// verifyRemoved - Verify that the value is removed from all internal data
/// structures.
void GVNPass::ValueTable::verifyRemoved(const Value *V) const {
- assert(!ValueNumbering.contains(V) &&
- "Inst still occurs in value numbering map!");
+ if (V != nullptr)
+ assert(!ValueNumbering.contains(V) &&
+ "Inst still occurs in value numbering map!");
}
//===----------------------------------------------------------------------===//
@@ -1572,11 +1572,11 @@ void GVNPass::eliminatePartiallyRedundantLoad(
I->setDebugLoc(Load->getDebugLoc());
if (V->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(V);
- markInstructionForDeletion(Load);
ORE->emit([&]() {
return OptimizationRemark(DEBUG_TYPE, "LoadPRE", Load)
<< "load eliminated by PRE";
});
+ doInstructionDeletion(Load);
}
bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
@@ -1795,7 +1795,7 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
// Erase instructions generated by the failed PHI translation before
// trying to number them. PHI translation might insert instructions
// in basic blocks other than the current one, and we delete them
- // directly, as markInstructionForDeletion only allows removing from the
+ // directly, as doInstructionDeletion only allows removing from the
// current basic block.
NewInsts.pop_back_val()->eraseFromParent();
}
@@ -1994,7 +1994,7 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) {
I->setDebugLoc(Load->getDebugLoc());
if (V->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(V);
- markInstructionForDeletion(Load);
+ doInstructionDeletion(Load);
++NumGVNLoad;
reportLoadElim(Load, V, ORE);
return true;
@@ -2064,7 +2064,7 @@ bool GVNPass::processAssumeIntrinsic(AssumeInst *IntrinsicI) {
}
}
if (isAssumeWithEmptyBundle(*IntrinsicI)) {
- markInstructionForDeletion(IntrinsicI);
+ doInstructionDeletion(IntrinsicI);
return true;
}
return false;
@@ -2175,7 +2175,7 @@ bool GVNPass::processLoad(LoadInst *L) {
return false;
if (L->use_empty()) {
- markInstructionForDeletion(L);
+ doInstructionDeletion(L);
return true;
}
@@ -2205,13 +2205,13 @@ bool GVNPass::processLoad(LoadInst *L) {
// MaterializeAdjustedValue is responsible for combining metadata.
ICF->removeUsersOf(L);
L->replaceAllUsesWith(AvailableValue);
- markInstructionForDeletion(L);
if (MSSAU)
MSSAU->removeMemoryAccess(L);
++NumGVNLoad;
reportLoadElim(L, AvailableValue, ORE);
// Tell MDA to reexamine the reused pointer since we might have more
// information after forwarding it.
+ doInstructionDeletion(L);
if (MD && AvailableValue->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(AvailableValue);
return true;
@@ -2601,7 +2601,7 @@ bool GVNPass::processInstruction(Instruction *I) {
Changed = true;
}
if (isInstructionTriviallyDead(I, TLI)) {
- markInstructionForDeletion(I);
+ doInstructionDeletion(I);
Changed = true;
}
if (Changed) {
@@ -2718,7 +2718,7 @@ bool GVNPass::processInstruction(Instruction *I) {
patchAndReplaceAllUsesWith(I, Repl);
if (MD && Repl->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(Repl);
- markInstructionForDeletion(I);
+ doInstructionDeletion(I);
return true;
}
@@ -2794,10 +2794,6 @@ bool GVNPass::runImpl(Function &F, AssumptionCache &RunAC, DominatorTree &RunDT,
}
bool GVNPass::processBlock(BasicBlock *BB) {
- // FIXME: Kill off InstrsToErase by doing erasing eagerly in a helper function
- // (and incrementing BI before processing an instruction).
- assert(InstrsToErase.empty() &&
- "We expect InstrsToErase to be empty across iterations");
if (DeadBlocks.count(BB))
return false;
@@ -2815,41 +2811,11 @@ bool GVNPass::processBlock(BasicBlock *BB) {
VN.erase(PN);
removeInstruction(PN);
}
-
- for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();
- BI != BE;) {
+ for (Instruction &Inst : make_early_inc_range(*BB)) {
if (!ReplaceOperandsWithMap.empty())
- ChangedFunction |= replaceOperandsForInBlockEquality(&*BI);
- ChangedFunction |= processInstruction(&*BI);
-
- if (InstrsToErase.empty()) {
- ++BI;
- continue;
- }
-
- // If we need some instructions deleted, do it now.
- NumGVNInstr += InstrsToErase.size();
-
- // Avoid iterator invalidation.
- bool AtStart = BI == BB->begin();
- if (!AtStart)
- --BI;
-
- for (auto *I : InstrsToErase) {
- assert(I->getParent() == BB && "Removing instruction from wrong block?");
- LLVM_DEBUG(dbgs() << "GVN removed: " << *I << '\n');
- salvageKnowledge(I, AC);
- salvageDebugInfo(*I);
- removeInstruction(I);
- }
- InstrsToErase.clear();
-
- if (AtStart)
- BI = BB->begin();
- else
- ++BI;
+ ChangedFunction |= replaceOperandsForInBlockEquality(&Inst);
+ ChangedFunction |= processInstruction(&Inst);
}
-
return ChangedFunction;
}
>From 7b358ed38a4119b8aa78b6bb406bf13fc1ad6c03 Mon Sep 17 00:00:00 2001
From: Madhur Amilkanthwar <madhura at nvidia.com>
Date: Sun, 20 Apr 2025 06:11:18 -0700
Subject: [PATCH 2/3] Address review comments
---
llvm/include/llvm/Transforms/Scalar/GVN.h | 7 +------
llvm/lib/Transforms/Scalar/GVN.cpp | 17 ++++++++++-------
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h
index 7aa43a467382e..ec03ad3433a8b 100644
--- a/llvm/include/llvm/Transforms/Scalar/GVN.h
+++ b/llvm/include/llvm/Transforms/Scalar/GVN.h
@@ -139,12 +139,7 @@ class GVNPass : public PassInfoMixin<GVNPass> {
/// This removes the specified instruction from
/// our various maps and marks it for deletion.
- void doInstructionDeletion(Instruction *I) {
- salvageKnowledge(I, AC);
- salvageDebugInfo(*I);
- VN.erase(I);
- removeInstruction(I);
- }
+ void doInstructionDeletion(Instruction *I);
DominatorTree &getDominatorTree() const { return *DT; }
AAResults *getAliasAnalysis() const { return VN.getAliasAnalysis(); }
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 0ea9c3c2f7e75..8234e430254e0 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -727,9 +727,8 @@ void GVNPass::ValueTable::erase(Value *V) {
/// verifyRemoved - Verify that the value is removed from all internal data
/// structures.
void GVNPass::ValueTable::verifyRemoved(const Value *V) const {
- if (V != nullptr)
- assert(!ValueNumbering.contains(V) &&
- "Inst still occurs in value numbering map!");
+ assert(!ValueNumbering.contains(V) &&
+ "Inst still occurs in value numbering map!");
}
//===----------------------------------------------------------------------===//
@@ -876,6 +875,12 @@ void GVNPass::printPipeline(
OS << '>';
}
+void GVNPass::doInstructionDeletion(Instruction *I) {
+ salvageKnowledge(I, AC);
+ salvageDebugInfo(*I);
+ removeInstruction(I);
+}
+
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
LLVM_DUMP_METHOD void GVNPass::dump(DenseMap<uint32_t, Value *> &Map) const {
errs() << "{\n";
@@ -1555,7 +1560,6 @@ void GVNPass::eliminatePartiallyRedundantLoad(
replaceValuesPerBlockEntry(ValuesPerBlock, OldLoad, NewLoad);
if (uint32_t ValNo = VN.lookup(OldLoad, false))
LeaderTable.erase(ValNo, OldLoad, OldLoad->getParent());
- VN.erase(OldLoad);
removeInstruction(OldLoad);
}
}
@@ -1994,9 +1998,9 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) {
I->setDebugLoc(Load->getDebugLoc());
if (V->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(V);
- doInstructionDeletion(Load);
++NumGVNLoad;
reportLoadElim(Load, V, ORE);
+ doInstructionDeletion(Load);
return true;
}
@@ -2808,7 +2812,6 @@ bool GVNPass::processBlock(BasicBlock *BB) {
SmallPtrSet<PHINode *, 8> PHINodesToRemove;
ChangedFunction |= EliminateDuplicatePHINodes(BB, PHINodesToRemove);
for (PHINode *PN : PHINodesToRemove) {
- VN.erase(PN);
removeInstruction(PN);
}
for (Instruction &Inst : make_early_inc_range(*BB)) {
@@ -3021,7 +3024,6 @@ bool GVNPass::performScalarPRE(Instruction *CurInst) {
CurInst->replaceAllUsesWith(Phi);
if (MD && Phi->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(Phi);
- VN.erase(CurInst);
LeaderTable.erase(ValNo, CurInst, CurrentBlock);
LLVM_DEBUG(dbgs() << "GVN PRE removed: " << *CurInst << '\n');
@@ -3121,6 +3123,7 @@ void GVNPass::cleanupGlobalSets() {
}
void GVNPass::removeInstruction(Instruction *I) {
+ VN.erase(I);
if (MD) MD->removeInstruction(I);
if (MSSAU)
MSSAU->removeMemoryAccess(I);
>From 738735b81ecea9717e06972b56bc3fe67628d003 Mon Sep 17 00:00:00 2001
From: Madhur Amilkanthwar <madhura at nvidia.com>
Date: Tue, 29 Apr 2025 00:55:30 -0700
Subject: [PATCH 3/3] Address review comments
---
llvm/include/llvm/Transforms/Scalar/GVN.h | 2 +-
llvm/lib/Transforms/Scalar/GVN.cpp | 18 +++++++++---------
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h
index ec03ad3433a8b..33a51bd121023 100644
--- a/llvm/include/llvm/Transforms/Scalar/GVN.h
+++ b/llvm/include/llvm/Transforms/Scalar/GVN.h
@@ -139,7 +139,7 @@ class GVNPass : public PassInfoMixin<GVNPass> {
/// This removes the specified instruction from
/// our various maps and marks it for deletion.
- void doInstructionDeletion(Instruction *I);
+ void salvageAndRemoveInstruction(Instruction *I);
DominatorTree &getDominatorTree() const { return *DT; }
AAResults *getAliasAnalysis() const { return VN.getAliasAnalysis(); }
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 8234e430254e0..27a4a469958f8 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -875,7 +875,7 @@ void GVNPass::printPipeline(
OS << '>';
}
-void GVNPass::doInstructionDeletion(Instruction *I) {
+void GVNPass::salvageAndRemoveInstruction(Instruction *I) {
salvageKnowledge(I, AC);
salvageDebugInfo(*I);
removeInstruction(I);
@@ -1580,7 +1580,7 @@ void GVNPass::eliminatePartiallyRedundantLoad(
return OptimizationRemark(DEBUG_TYPE, "LoadPRE", Load)
<< "load eliminated by PRE";
});
- doInstructionDeletion(Load);
+ salvageAndRemoveInstruction(Load);
}
bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
@@ -1799,7 +1799,7 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
// Erase instructions generated by the failed PHI translation before
// trying to number them. PHI translation might insert instructions
// in basic blocks other than the current one, and we delete them
- // directly, as doInstructionDeletion only allows removing from the
+ // directly, as salvageAndRemoveInstruction only allows removing from the
// current basic block.
NewInsts.pop_back_val()->eraseFromParent();
}
@@ -2000,7 +2000,7 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) {
MD->invalidateCachedPointerInfo(V);
++NumGVNLoad;
reportLoadElim(Load, V, ORE);
- doInstructionDeletion(Load);
+ salvageAndRemoveInstruction(Load);
return true;
}
@@ -2068,7 +2068,7 @@ bool GVNPass::processAssumeIntrinsic(AssumeInst *IntrinsicI) {
}
}
if (isAssumeWithEmptyBundle(*IntrinsicI)) {
- doInstructionDeletion(IntrinsicI);
+ salvageAndRemoveInstruction(IntrinsicI);
return true;
}
return false;
@@ -2179,7 +2179,7 @@ bool GVNPass::processLoad(LoadInst *L) {
return false;
if (L->use_empty()) {
- doInstructionDeletion(L);
+ salvageAndRemoveInstruction(L);
return true;
}
@@ -2213,9 +2213,9 @@ bool GVNPass::processLoad(LoadInst *L) {
MSSAU->removeMemoryAccess(L);
++NumGVNLoad;
reportLoadElim(L, AvailableValue, ORE);
+ salvageAndRemoveInstruction(L);
// Tell MDA to reexamine the reused pointer since we might have more
// information after forwarding it.
- doInstructionDeletion(L);
if (MD && AvailableValue->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(AvailableValue);
return true;
@@ -2605,7 +2605,7 @@ bool GVNPass::processInstruction(Instruction *I) {
Changed = true;
}
if (isInstructionTriviallyDead(I, TLI)) {
- doInstructionDeletion(I);
+ salvageAndRemoveInstruction(I);
Changed = true;
}
if (Changed) {
@@ -2722,7 +2722,7 @@ bool GVNPass::processInstruction(Instruction *I) {
patchAndReplaceAllUsesWith(I, Repl);
if (MD && Repl->getType()->isPtrOrPtrVectorTy())
MD->invalidateCachedPointerInfo(Repl);
- doInstructionDeletion(I);
+ salvageAndRemoveInstruction(I);
return true;
}
More information about the llvm-commits
mailing list