From a54e0a5f1b281a5999ad0e68eea9a94012e74a01 Mon Sep 17 00:00:00 2001 From: Madhur Amilkanthwar 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 #include #include @@ -137,9 +139,11 @@ class GVNPass : public PassInfoMixin { /// 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 { // propagate to any successors. Entries added mid-block are applied // to the remaining instructions in the block. SmallMapVector ReplaceOperandsWithMap; - SmallVector 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 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 { /// 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 &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 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 9ad0eaf186c4648d279e2108e98bc89240f82b91 Mon Sep 17 00:00:00 2001 From: Madhur Amilkanthwar Date: Tue, 29 Apr 2025 00:55:30 -0700 Subject: [PATCH 3/3] Address review comments --- llvm/include/llvm/Transforms/Scalar/GVN.h | 4 +--- llvm/lib/Transforms/Scalar/GVN.cpp | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h index ec03ad3433a8b..25b042eebf664 100644 --- a/llvm/include/llvm/Transforms/Scalar/GVN.h +++ b/llvm/include/llvm/Transforms/Scalar/GVN.h @@ -25,8 +25,6 @@ #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 #include #include @@ -139,7 +137,7 @@ class GVNPass : public PassInfoMixin { /// 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..5d5ab9eca448e 100644 --- a/llvm/lib/Transforms/Scalar/GVN.cpp +++ b/llvm/lib/Transforms/Scalar/GVN.cpp @@ -69,6 +69,7 @@ #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" @@ -875,7 +876,7 @@ void GVNPass::printPipeline( OS << '>'; } -void GVNPass::doInstructionDeletion(Instruction *I) { +void GVNPass::salvageAndRemoveInstruction(Instruction *I) { salvageKnowledge(I, AC); salvageDebugInfo(*I); removeInstruction(I); @@ -1580,7 +1581,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 +1800,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 +2001,7 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) { MD->invalidateCachedPointerInfo(V); ++NumGVNLoad; reportLoadElim(Load, V, ORE); - doInstructionDeletion(Load); + salvageAndRemoveInstruction(Load); return true; } @@ -2068,7 +2069,7 @@ bool GVNPass::processAssumeIntrinsic(AssumeInst *IntrinsicI) { } } if (isAssumeWithEmptyBundle(*IntrinsicI)) { - doInstructionDeletion(IntrinsicI); + salvageAndRemoveInstruction(IntrinsicI); return true; } return false; @@ -2179,7 +2180,7 @@ bool GVNPass::processLoad(LoadInst *L) { return false; if (L->use_empty()) { - doInstructionDeletion(L); + salvageAndRemoveInstruction(L); return true; } @@ -2213,9 +2214,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 +2606,7 @@ bool GVNPass::processInstruction(Instruction *I) { Changed = true; } if (isInstructionTriviallyDead(I, TLI)) { - doInstructionDeletion(I); + salvageAndRemoveInstruction(I); Changed = true; } if (Changed) { @@ -2722,7 +2723,7 @@ bool GVNPass::processInstruction(Instruction *I) { patchAndReplaceAllUsesWith(I, Repl); if (MD && Repl->getType()->isPtrOrPtrVectorTy()) MD->invalidateCachedPointerInfo(Repl); - doInstructionDeletion(I); + salvageAndRemoveInstruction(I); return true; }