[VPlan] Manage created blocks directly in VPlan. (NFC) #120918
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-vectorizers Author: Florian Hahn (fhahn) ChangesThis patch changes the way blocks are managed by VPlan. Previously all blocks reachable from entry would be cleaned up when a VPlan is destroyed. With this patch, each VPlan keeps track of blocks created for it in a list and this list is then used to delete all blocks in the list when the VPlan is destroyed. To do so, block creation is funneled through helpers in directly in VPlan. The main advantage of doing so is it simplifies CFG transformations, as those do not have to take care of deleting any blocks, just adjusting the CFG. This helps to simplify This also simplifies handling of 'immutable' blocks a VPlan holds references to, which at the moment only include the scalar header block. Note that the original constructors taking VPBlockBase are retained at the moment for unit tests. Full diff: https://github.com/llvm/llvm-project/pull/120918.diff 5 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 1f6996cd9c1f49..8e832fdf9fe879 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -2477,7 +2477,7 @@ static void introduceCheckBlockInVPlan(VPlan &Plan, BasicBlock *CheckIRBB) {
assert(PreVectorPH->getNumSuccessors() == 2 && "Expected 2 successors");
assert(PreVectorPH->getSuccessors()[0] == ScalarPH &&
"Unexpected successor");
- VPIRBasicBlock *CheckVPIRBB = VPIRBasicBlock::fromBasicBlock(CheckIRBB);
+ VPIRBasicBlock *CheckVPIRBB = Plan.createVPIRBasicBlock(CheckIRBB);
VPBlockUtils::insertOnEdge(PreVectorPH, VectorPH, CheckVPIRBB);
PreVectorPH = CheckVPIRBB;
}
@@ -8185,11 +8185,10 @@ EpilogueVectorizerEpilogueLoop::emitMinimumVectorEpilogueIterCountCheck(
// A new entry block has been created for the epilogue VPlan. Hook it in, as
// otherwise we would try to modify the entry to the main vector loop.
- VPIRBasicBlock *NewEntry = VPIRBasicBlock::fromBasicBlock(Insert);
+ VPIRBasicBlock *NewEntry = Plan.createVPIRBasicBlock(Insert);
VPBasicBlock *OldEntry = Plan.getEntry();
VPBlockUtils::reassociateBlocks(OldEntry, NewEntry);
Plan.setEntry(NewEntry);
- delete OldEntry;
introduceCheckBlockInVPlan(Plan, Insert);
return Insert;
@@ -9335,7 +9334,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
VPBB->appendRecipe(Recipe);
}
- VPBlockUtils::insertBlockAfter(new VPBasicBlock(), VPBB);
+ VPBlockUtils::insertBlockAfter(Plan->createVPBasicBlock(""), VPBB);
VPBB = cast<VPBasicBlock>(VPBB->getSingleSuccessor());
}
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 71f43abe534ec0..f7f511207a8ffa 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -205,11 +205,6 @@ VPBlockBase *VPBlockBase::getEnclosingBlockWithPredecessors() {
return Parent->getEnclosingBlockWithPredecessors();
}
-void VPBlockBase::deleteCFG(VPBlockBase *Entry) {
- for (VPBlockBase *Block : to_vector(vp_depth_first_shallow(Entry)))
- delete Block;
-}
-
VPBasicBlock::iterator VPBasicBlock::getFirstNonPhi() {
iterator It = begin();
while (It != end() && It->isPhi())
@@ -474,6 +469,16 @@ void VPIRBasicBlock::execute(VPTransformState *State) {
connectToPredecessors(State->CFG);
}
+VPIRBasicBlock *VPIRBasicBlock::clone() {
+ auto *NewBlock = getPlan()->createVPIRBasicBlock(IRBB);
+ for (VPRecipeBase &R : make_early_inc_range(*NewBlock))
+ R.eraseFromParent();
+
+ for (VPRecipeBase &R : Recipes)
+ NewBlock->appendRecipe(R.clone());
+ return NewBlock;
+}
+
void VPBasicBlock::execute(VPTransformState *State) {
bool Replica = bool(State->Lane);
BasicBlock *NewBB = State->CFG.PrevBB; // Reuse it if possible.
@@ -523,6 +528,13 @@ void VPBasicBlock::dropAllReferences(VPValue *NewValue) {
}
}
+VPBasicBlock *VPBasicBlock::clone() {
+ auto *NewBlock = getPlan()->createVPBasicBlock(getName());
+ for (VPRecipeBase &R : *this)
+ NewBlock->appendRecipe(R.clone());
+ return NewBlock;
+}
+
void VPBasicBlock::executeRecipes(VPTransformState *State, BasicBlock *BB) {
LLVM_DEBUG(dbgs() << "LV: vectorizing VPBB:" << getName()
<< " in BB:" << BB->getName() << '\n');
@@ -541,7 +553,7 @@ VPBasicBlock *VPBasicBlock::splitAt(iterator SplitAt) {
SmallVector<VPBlockBase *, 2> Succs(successors());
// Create new empty block after the block to split.
- auto *SplitBlock = new VPBasicBlock(getName() + ".split");
+ auto *SplitBlock = getPlan()->createVPBasicBlock(getName() + ".split");
VPBlockUtils::insertBlockAfter(SplitBlock, this);
// Finally, move the recipes starting at SplitAt to new block.
@@ -701,8 +713,8 @@ static std::pair<VPBlockBase *, VPBlockBase *> cloneFrom(VPBlockBase *Entry) {
VPRegionBlock *VPRegionBlock::clone() {
const auto &[NewEntry, NewExiting] = cloneFrom(getEntry());
- auto *NewRegion =
- new VPRegionBlock(NewEntry, NewExiting, getName(), isReplicator());
+ auto *NewRegion = getPlan()->createVPRegionBlock(NewEntry, NewExiting,
+ getName(), isReplicator());
for (VPBlockBase *Block : vp_depth_first_shallow(NewEntry))
Block->setParent(NewRegion);
return NewRegion;
@@ -822,17 +834,20 @@ void VPRegionBlock::print(raw_ostream &O, const Twine &Indent,
#endif
VPlan::VPlan(Loop *L) {
- setEntry(VPIRBasicBlock::fromBasicBlock(L->getLoopPreheader()));
- ScalarHeader = VPIRBasicBlock::fromBasicBlock(L->getHeader());
+ setEntry(createVPIRBasicBlock(L->getLoopPreheader()));
+ ScalarHeader = createVPIRBasicBlock(L->getHeader());
}
VPlan::~VPlan() {
if (Entry) {
VPValue DummyValue;
- for (VPBlockBase *Block : vp_depth_first_shallow(Entry))
- Block->dropAllReferences(&DummyValue);
- VPBlockBase::deleteCFG(Entry);
+ for (auto *VPB : reverse(CreatedBlocks))
+ VPB->dropAllReferences(&DummyValue);
+
+ for (auto *VPB : reverse(CreatedBlocks)) {
+ delete VPB;
+ }
}
for (VPValue *VPV : VPLiveInsToFree)
delete VPV;
@@ -840,14 +855,6 @@ VPlan::~VPlan() {
delete BackedgeTakenCount;
}
-VPIRBasicBlock *VPIRBasicBlock::fromBasicBlock(BasicBlock *IRBB) {
- auto *VPIRBB = new VPIRBasicBlock(IRBB);
- for (Instruction &I :
- make_range(IRBB->begin(), IRBB->getTerminator()->getIterator()))
- VPIRBB->appendRecipe(new VPIRInstruction(I));
- return VPIRBB;
-}
-
VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
PredicatedScalarEvolution &PSE,
bool RequiresScalarEpilogueCheck,
@@ -861,7 +868,7 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// an epilogue vector loop, the original entry block here will be replaced by
// a new VPIRBasicBlock wrapping the entry to the epilogue vector loop after
// generating code for the main vector loop.
- VPBasicBlock *VecPreheader = new VPBasicBlock("vector.ph");
+ VPBasicBlock *VecPreheader = Plan->createVPBasicBlock("vector.ph");
VPBlockUtils::connectBlocks(Plan->getEntry(), VecPreheader);
// Create SCEV and VPValue for the trip count.
@@ -878,17 +885,17 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// Create VPRegionBlock, with empty header and latch blocks, to be filled
// during processing later.
- VPBasicBlock *HeaderVPBB = new VPBasicBlock("vector.body");
- VPBasicBlock *LatchVPBB = new VPBasicBlock("vector.latch");
+ VPBasicBlock *HeaderVPBB = Plan->createVPBasicBlock("vector.body");
+ VPBasicBlock *LatchVPBB = Plan->createVPBasicBlock("vector.latch");
VPBlockUtils::insertBlockAfter(LatchVPBB, HeaderVPBB);
- auto *TopRegion = new VPRegionBlock(HeaderVPBB, LatchVPBB, "vector loop",
- false /*isReplicator*/);
+ auto *TopRegion = Plan->createVPRegionBlock(
+ HeaderVPBB, LatchVPBB, "vector loop", false /*isReplicator*/);
VPBlockUtils::insertBlockAfter(TopRegion, VecPreheader);
- VPBasicBlock *MiddleVPBB = new VPBasicBlock("middle.block");
+ VPBasicBlock *MiddleVPBB = Plan->createVPBasicBlock("middle.block");
VPBlockUtils::insertBlockAfter(MiddleVPBB, TopRegion);
- VPBasicBlock *ScalarPH = new VPBasicBlock("scalar.ph");
+ VPBasicBlock *ScalarPH = Plan->createVPBasicBlock("scalar.ph");
VPBlockUtils::connectBlocks(ScalarPH, ScalarHeader);
if (!RequiresScalarEpilogueCheck) {
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
@@ -904,7 +911,7 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// we unconditionally branch to the scalar preheader. Do nothing.
// 3) Otherwise, construct a runtime check.
BasicBlock *IRExitBlock = TheLoop->getUniqueLatchExitBlock();
- auto *VPExitBlock = VPIRBasicBlock::fromBasicBlock(IRExitBlock);
+ auto *VPExitBlock = Plan->createVPIRBasicBlock(IRExitBlock);
// The connection order corresponds to the operands of the conditional branch.
VPBlockUtils::insertBlockAfter(VPExitBlock, MiddleVPBB);
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
@@ -960,15 +967,13 @@ void VPlan::prepareToExecute(Value *TripCountV, Value *VectorTripCountV,
/// have a single predecessor, which is rewired to the new VPIRBasicBlock. All
/// successors of VPBB, if any, are rewired to the new VPIRBasicBlock.
static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) {
- VPIRBasicBlock *IRVPBB = VPIRBasicBlock::fromBasicBlock(IRBB);
+ VPIRBasicBlock *IRVPBB = VPBB->getPlan()->createVPIRBasicBlock(IRBB);
for (auto &R : make_early_inc_range(*VPBB)) {
assert(!R.isPhi() && "Tried to move phi recipe to end of block");
R.moveBefore(*IRVPBB, IRVPBB->end());
}
VPBlockUtils::reassociateBlocks(VPBB, IRVPBB);
-
- delete VPBB;
}
/// Generate the code inside the preheader and body of the vectorized loop.
@@ -1222,6 +1227,7 @@ static void remapOperands(VPBlockBase *Entry, VPBlockBase *NewEntry,
}
VPlan *VPlan::duplicate() {
+ unsigned CreatedBlockSize = CreatedBlocks.size();
// Clone blocks.
const auto &[NewEntry, __] = cloneFrom(Entry);
@@ -1262,9 +1268,23 @@ VPlan *VPlan::duplicate() {
assert(Old2NewVPValues.contains(TripCount) &&
"TripCount must have been added to Old2NewVPValues");
NewPlan->TripCount = Old2NewVPValues[TripCount];
+
+ for (unsigned I = CreatedBlockSize; I != CreatedBlocks.size(); ++I)
+ NewPlan->CreatedBlocks.push_back(CreatedBlocks[I]);
+ CreatedBlocks.truncate(CreatedBlockSize);
+
return NewPlan;
}
+VPIRBasicBlock *VPlan::createVPIRBasicBlock(BasicBlock *IRBB) {
+ auto *VPIRBB = new VPIRBasicBlock(IRBB);
+ for (Instruction &I :
+ make_range(IRBB->begin(), IRBB->getTerminator()->getIterator()))
+ VPIRBB->appendRecipe(new VPIRInstruction(I));
+ CreatedBlocks.push_back(VPIRBB);
+ return VPIRBB;
+}
+
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Twine VPlanPrinter::getUID(const VPBlockBase *Block) {
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 6486c6745a6800..9193c9e873ff3c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -636,9 +636,6 @@ class VPBlockBase {
/// Return the cost of the block.
virtual InstructionCost cost(ElementCount VF, VPCostContext &Ctx) = 0;
- /// Delete all blocks reachable from a given VPBlockBase, inclusive.
- static void deleteCFG(VPBlockBase *Entry);
-
/// Return true if it is legal to hoist instructions into this block.
bool isLegalToHoistInto() {
// There are currently no constraints that prevent an instruction to be
@@ -3585,12 +3582,7 @@ class VPBasicBlock : public VPBlockBase {
/// Clone the current block and it's recipes, without updating the operands of
/// the cloned recipes.
- VPBasicBlock *clone() override {
- auto *NewBlock = new VPBasicBlock(getName());
- for (VPRecipeBase &R : *this)
- NewBlock->appendRecipe(R.clone());
- return NewBlock;
- }
+ VPBasicBlock *clone() override;
protected:
/// Execute the recipes in the IR basic block \p BB.
@@ -3626,20 +3618,11 @@ class VPIRBasicBlock : public VPBasicBlock {
return V->getVPBlockID() == VPBlockBase::VPIRBasicBlockSC;
}
- /// Create a VPIRBasicBlock from \p IRBB containing VPIRInstructions for all
- /// instructions in \p IRBB, except its terminator which is managed in VPlan.
- static VPIRBasicBlock *fromBasicBlock(BasicBlock *IRBB);
-
/// The method which generates the output IR instructions that correspond to
/// this VPBasicBlock, thereby "executing" the VPlan.
void execute(VPTransformState *State) override;
- VPIRBasicBlock *clone() override {
- auto *NewBlock = new VPIRBasicBlock(IRBB);
- for (VPRecipeBase &R : Recipes)
- NewBlock->appendRecipe(R.clone());
- return NewBlock;
- }
+ VPIRBasicBlock *clone() override;
BasicBlock *getIRBasicBlock() const { return IRBB; }
};
@@ -3679,11 +3662,6 @@ class VPRegionBlock : public VPBlockBase {
IsReplicator(IsReplicator) {}
~VPRegionBlock() override {
- if (Entry) {
- VPValue DummyValue;
- Entry->dropAllReferences(&DummyValue);
- deleteCFG(Entry);
- }
}
/// Method to support type inquiry through isa, cast, and dyn_cast.
@@ -3810,6 +3788,8 @@ class VPlan {
/// been modeled in VPlan directly.
DenseMap<const SCEV *, VPValue *> SCEVToExpansion;
+ SmallVector<VPBlockBase *> CreatedBlocks;
+
public:
/// Construct a VPlan with \p Entry to the plan and with \p ScalarHeader
/// wrapping the original header of the scalar loop.
@@ -4026,6 +4006,32 @@ class VPlan {
/// Clone the current VPlan, update all VPValues of the new VPlan and cloned
/// recipes to refer to the clones, and return it.
VPlan *duplicate();
+
+ VPBasicBlock *createVPBasicBlock(const Twine &Name,
+ VPRecipeBase *Recipe = nullptr) {
+ auto *VPB = new VPBasicBlock(Name, Recipe);
+ CreatedBlocks.push_back(VPB);
+ return VPB;
+ }
+
+ VPRegionBlock *createVPRegionBlock(VPBlockBase *Entry, VPBlockBase *Exiting,
+ const std::string &Name = "",
+ bool IsReplicator = false) {
+ auto *VPB = new VPRegionBlock(Entry, Exiting, Name, IsReplicator);
+ CreatedBlocks.push_back(VPB);
+ return VPB;
+ }
+
+ VPRegionBlock *createVPRegionBlock(const std::string &Name = "",
+ bool IsReplicator = false) {
+ auto *VPB = new VPRegionBlock(Name, IsReplicator);
+ CreatedBlocks.push_back(VPB);
+ return VPB;
+ }
+
+ /// Create a VPIRBasicBlock from \p IRBB containing VPIRInstructions for all
+ /// instructions in \p IRBB, except its terminator which is managed in VPlan.
+ VPIRBasicBlock *createVPIRBasicBlock(BasicBlock *IRBB);
};
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 6e633739fcc3dd..02f4c8d8872d82 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -182,7 +182,7 @@ VPBasicBlock *PlainCFGBuilder::getOrCreateVPBB(BasicBlock *BB) {
// Create new VPBB.
StringRef Name = isHeaderBB(BB, TheLoop) ? "vector.body" : BB->getName();
LLVM_DEBUG(dbgs() << "Creating VPBasicBlock for " << Name << "\n");
- VPBasicBlock *VPBB = new VPBasicBlock(Name);
+ VPBasicBlock *VPBB = Plan.createVPBasicBlock(Name);
BB2VPBB[BB] = VPBB;
// Get or create a region for the loop containing BB.
@@ -204,7 +204,7 @@ VPBasicBlock *PlainCFGBuilder::getOrCreateVPBB(BasicBlock *BB) {
if (LoopOfBB == TheLoop) {
RegionOfVPBB = Plan.getVectorLoopRegion();
} else {
- RegionOfVPBB = new VPRegionBlock(Name.str(), false /*isReplicator*/);
+ RegionOfVPBB = Plan.createVPRegionBlock(Name.str(), false /*isReplicator*/);
RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
}
RegionOfVPBB->setEntry(VPBB);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 43c7a935e2f1bc..d672f591abce98 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -297,8 +297,6 @@ static bool mergeReplicateRegionsIntoSuccessors(VPlan &Plan) {
DeletedRegions.insert(Region1);
}
- for (VPRegionBlock *ToDelete : DeletedRegions)
- delete ToDelete;
return !DeletedRegions.empty();
}
@@ -310,7 +308,8 @@ static VPRegionBlock *createReplicateRegion(VPReplicateRecipe *PredRecipe,
assert(Instr->getParent() && "Predicated instruction not in any basic block");
auto *BlockInMask = PredRecipe->getMask();
auto *BOMRecipe = new VPBranchOnMaskRecipe(BlockInMask);
- auto *Entry = new VPBasicBlock(Twine(RegionName) + ".entry", BOMRecipe);
+ auto *Entry =
+ Plan.createVPBasicBlock(Twine(RegionName) + ".entry", BOMRecipe);
// Replace predicated replicate recipe with a replicate recipe without a
// mask but in the replicate region.
@@ -318,7 +317,8 @@ static VPRegionBlock *createReplicateRegion(VPReplicateRecipe *PredRecipe,
PredRecipe->getUnderlyingInstr(),
make_range(PredRecipe->op_begin(), std::prev(PredRecipe->op_end())),
PredRecipe->isUniform());
- auto *Pred = new VPBasicBlock(Twine(RegionName) + ".if", RecipeWithoutMask);
+ auto *Pred =
+ Plan.createVPBasicBlock(Twine(RegionName) + ".if", RecipeWithoutMask);
VPPredInstPHIRecipe *PHIRecipe = nullptr;
if (PredRecipe->getNumUsers() != 0) {
@@ -328,8 +328,10 @@ static VPRegionBlock *createReplicateRegion(VPReplicateRecipe *PredRecipe,
PHIRecipe->setOperand(0, RecipeWithoutMask);
}
PredRecipe->eraseFromParent();
- auto *Exiting = new VPBasicBlock(Twine(RegionName) + ".continue", PHIRecipe);
- VPRegionBlock *Region = new VPRegionBlock(Entry, Exiting, RegionName, true);
+ auto *Exiting =
+ Plan.createVPBasicBlock(Twine(RegionName) + ".continue", PHIRecipe);
+ VPRegionBlock *Region =
+ Plan.createVPRegionBlock(Entry, Exiting, RegionName, true);
// Note: first set Entry as region entry and then connect successors starting
// from it in order, to propagate the "parent" of each VPBasicBlock.
@@ -396,7 +398,6 @@ static bool mergeBlocksIntoPredecessors(VPlan &Plan) {
VPBlockUtils::disconnectBlocks(VPBB, Succ);
VPBlockUtils::connectBlocks(PredVPBB, Succ);
}
- delete VPBB;
}
return !WorkList.empty();
}
@@ -1904,7 +1905,7 @@ void VPlanTransforms::handleUncountableEarlyExit(
if (OrigLoop->getUniqueExitBlock()) {
VPEarlyExitBlock = cast<VPIRBasicBlock>(MiddleVPBB->getSuccessors()[0]);
} else {
- VPEarlyExitBlock = VPIRBasicBlock::fromBasicBlock(
+ VPEarlyExitBlock = Plan.createVPIRBasicBlock(
!OrigLoop->contains(TrueSucc) ? TrueSucc : FalseSucc);
}
@@ -1914,7 +1915,7 @@ void VPlanTransforms::handleUncountableEarlyExit(
IsEarlyExitTaken =
Builder.createNaryOp(VPInstruction::AnyOf, {EarlyExitTakenCond});
- VPBasicBlock *NewMiddle = new VPBasicBlock("middle.split");
+ VPBasicBlock *NewMiddle = Plan.createVPBasicBlock("middle.split");
VPBlockUtils::insertOnEdge(LoopRegion, MiddleVPBB, NewMiddle);
VPBlockUtils::connectBlocks(NewMiddle, VPEarlyExitBlock);
NewMiddle->swapSuccessors();
|
|
@llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesThis patch changes the way blocks are managed by VPlan. Previously all blocks reachable from entry would be cleaned up when a VPlan is destroyed. With this patch, each VPlan keeps track of blocks created for it in a list and this list is then used to delete all blocks in the list when the VPlan is destroyed. To do so, block creation is funneled through helpers in directly in VPlan. The main advantage of doing so is it simplifies CFG transformations, as those do not have to take care of deleting any blocks, just adjusting the CFG. This helps to simplify This also simplifies handling of 'immutable' blocks a VPlan holds references to, which at the moment only include the scalar header block. Note that the original constructors taking VPBlockBase are retained at the moment for unit tests. Full diff: https://github.com/llvm/llvm-project/pull/120918.diff 5 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 1f6996cd9c1f49..8e832fdf9fe879 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -2477,7 +2477,7 @@ static void introduceCheckBlockInVPlan(VPlan &Plan, BasicBlock *CheckIRBB) {
assert(PreVectorPH->getNumSuccessors() == 2 && "Expected 2 successors");
assert(PreVectorPH->getSuccessors()[0] == ScalarPH &&
"Unexpected successor");
- VPIRBasicBlock *CheckVPIRBB = VPIRBasicBlock::fromBasicBlock(CheckIRBB);
+ VPIRBasicBlock *CheckVPIRBB = Plan.createVPIRBasicBlock(CheckIRBB);
VPBlockUtils::insertOnEdge(PreVectorPH, VectorPH, CheckVPIRBB);
PreVectorPH = CheckVPIRBB;
}
@@ -8185,11 +8185,10 @@ EpilogueVectorizerEpilogueLoop::emitMinimumVectorEpilogueIterCountCheck(
// A new entry block has been created for the epilogue VPlan. Hook it in, as
// otherwise we would try to modify the entry to the main vector loop.
- VPIRBasicBlock *NewEntry = VPIRBasicBlock::fromBasicBlock(Insert);
+ VPIRBasicBlock *NewEntry = Plan.createVPIRBasicBlock(Insert);
VPBasicBlock *OldEntry = Plan.getEntry();
VPBlockUtils::reassociateBlocks(OldEntry, NewEntry);
Plan.setEntry(NewEntry);
- delete OldEntry;
introduceCheckBlockInVPlan(Plan, Insert);
return Insert;
@@ -9335,7 +9334,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
VPBB->appendRecipe(Recipe);
}
- VPBlockUtils::insertBlockAfter(new VPBasicBlock(), VPBB);
+ VPBlockUtils::insertBlockAfter(Plan->createVPBasicBlock(""), VPBB);
VPBB = cast<VPBasicBlock>(VPBB->getSingleSuccessor());
}
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 71f43abe534ec0..f7f511207a8ffa 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -205,11 +205,6 @@ VPBlockBase *VPBlockBase::getEnclosingBlockWithPredecessors() {
return Parent->getEnclosingBlockWithPredecessors();
}
-void VPBlockBase::deleteCFG(VPBlockBase *Entry) {
- for (VPBlockBase *Block : to_vector(vp_depth_first_shallow(Entry)))
- delete Block;
-}
-
VPBasicBlock::iterator VPBasicBlock::getFirstNonPhi() {
iterator It = begin();
while (It != end() && It->isPhi())
@@ -474,6 +469,16 @@ void VPIRBasicBlock::execute(VPTransformState *State) {
connectToPredecessors(State->CFG);
}
+VPIRBasicBlock *VPIRBasicBlock::clone() {
+ auto *NewBlock = getPlan()->createVPIRBasicBlock(IRBB);
+ for (VPRecipeBase &R : make_early_inc_range(*NewBlock))
+ R.eraseFromParent();
+
+ for (VPRecipeBase &R : Recipes)
+ NewBlock->appendRecipe(R.clone());
+ return NewBlock;
+}
+
void VPBasicBlock::execute(VPTransformState *State) {
bool Replica = bool(State->Lane);
BasicBlock *NewBB = State->CFG.PrevBB; // Reuse it if possible.
@@ -523,6 +528,13 @@ void VPBasicBlock::dropAllReferences(VPValue *NewValue) {
}
}
+VPBasicBlock *VPBasicBlock::clone() {
+ auto *NewBlock = getPlan()->createVPBasicBlock(getName());
+ for (VPRecipeBase &R : *this)
+ NewBlock->appendRecipe(R.clone());
+ return NewBlock;
+}
+
void VPBasicBlock::executeRecipes(VPTransformState *State, BasicBlock *BB) {
LLVM_DEBUG(dbgs() << "LV: vectorizing VPBB:" << getName()
<< " in BB:" << BB->getName() << '\n');
@@ -541,7 +553,7 @@ VPBasicBlock *VPBasicBlock::splitAt(iterator SplitAt) {
SmallVector<VPBlockBase *, 2> Succs(successors());
// Create new empty block after the block to split.
- auto *SplitBlock = new VPBasicBlock(getName() + ".split");
+ auto *SplitBlock = getPlan()->createVPBasicBlock(getName() + ".split");
VPBlockUtils::insertBlockAfter(SplitBlock, this);
// Finally, move the recipes starting at SplitAt to new block.
@@ -701,8 +713,8 @@ static std::pair<VPBlockBase *, VPBlockBase *> cloneFrom(VPBlockBase *Entry) {
VPRegionBlock *VPRegionBlock::clone() {
const auto &[NewEntry, NewExiting] = cloneFrom(getEntry());
- auto *NewRegion =
- new VPRegionBlock(NewEntry, NewExiting, getName(), isReplicator());
+ auto *NewRegion = getPlan()->createVPRegionBlock(NewEntry, NewExiting,
+ getName(), isReplicator());
for (VPBlockBase *Block : vp_depth_first_shallow(NewEntry))
Block->setParent(NewRegion);
return NewRegion;
@@ -822,17 +834,20 @@ void VPRegionBlock::print(raw_ostream &O, const Twine &Indent,
#endif
VPlan::VPlan(Loop *L) {
- setEntry(VPIRBasicBlock::fromBasicBlock(L->getLoopPreheader()));
- ScalarHeader = VPIRBasicBlock::fromBasicBlock(L->getHeader());
+ setEntry(createVPIRBasicBlock(L->getLoopPreheader()));
+ ScalarHeader = createVPIRBasicBlock(L->getHeader());
}
VPlan::~VPlan() {
if (Entry) {
VPValue DummyValue;
- for (VPBlockBase *Block : vp_depth_first_shallow(Entry))
- Block->dropAllReferences(&DummyValue);
- VPBlockBase::deleteCFG(Entry);
+ for (auto *VPB : reverse(CreatedBlocks))
+ VPB->dropAllReferences(&DummyValue);
+
+ for (auto *VPB : reverse(CreatedBlocks)) {
+ delete VPB;
+ }
}
for (VPValue *VPV : VPLiveInsToFree)
delete VPV;
@@ -840,14 +855,6 @@ VPlan::~VPlan() {
delete BackedgeTakenCount;
}
-VPIRBasicBlock *VPIRBasicBlock::fromBasicBlock(BasicBlock *IRBB) {
- auto *VPIRBB = new VPIRBasicBlock(IRBB);
- for (Instruction &I :
- make_range(IRBB->begin(), IRBB->getTerminator()->getIterator()))
- VPIRBB->appendRecipe(new VPIRInstruction(I));
- return VPIRBB;
-}
-
VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
PredicatedScalarEvolution &PSE,
bool RequiresScalarEpilogueCheck,
@@ -861,7 +868,7 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// an epilogue vector loop, the original entry block here will be replaced by
// a new VPIRBasicBlock wrapping the entry to the epilogue vector loop after
// generating code for the main vector loop.
- VPBasicBlock *VecPreheader = new VPBasicBlock("vector.ph");
+ VPBasicBlock *VecPreheader = Plan->createVPBasicBlock("vector.ph");
VPBlockUtils::connectBlocks(Plan->getEntry(), VecPreheader);
// Create SCEV and VPValue for the trip count.
@@ -878,17 +885,17 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// Create VPRegionBlock, with empty header and latch blocks, to be filled
// during processing later.
- VPBasicBlock *HeaderVPBB = new VPBasicBlock("vector.body");
- VPBasicBlock *LatchVPBB = new VPBasicBlock("vector.latch");
+ VPBasicBlock *HeaderVPBB = Plan->createVPBasicBlock("vector.body");
+ VPBasicBlock *LatchVPBB = Plan->createVPBasicBlock("vector.latch");
VPBlockUtils::insertBlockAfter(LatchVPBB, HeaderVPBB);
- auto *TopRegion = new VPRegionBlock(HeaderVPBB, LatchVPBB, "vector loop",
- false /*isReplicator*/);
+ auto *TopRegion = Plan->createVPRegionBlock(
+ HeaderVPBB, LatchVPBB, "vector loop", false /*isReplicator*/);
VPBlockUtils::insertBlockAfter(TopRegion, VecPreheader);
- VPBasicBlock *MiddleVPBB = new VPBasicBlock("middle.block");
+ VPBasicBlock *MiddleVPBB = Plan->createVPBasicBlock("middle.block");
VPBlockUtils::insertBlockAfter(MiddleVPBB, TopRegion);
- VPBasicBlock *ScalarPH = new VPBasicBlock("scalar.ph");
+ VPBasicBlock *ScalarPH = Plan->createVPBasicBlock("scalar.ph");
VPBlockUtils::connectBlocks(ScalarPH, ScalarHeader);
if (!RequiresScalarEpilogueCheck) {
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
@@ -904,7 +911,7 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// we unconditionally branch to the scalar preheader. Do nothing.
// 3) Otherwise, construct a runtime check.
BasicBlock *IRExitBlock = TheLoop->getUniqueLatchExitBlock();
- auto *VPExitBlock = VPIRBasicBlock::fromBasicBlock(IRExitBlock);
+ auto *VPExitBlock = Plan->createVPIRBasicBlock(IRExitBlock);
// The connection order corresponds to the operands of the conditional branch.
VPBlockUtils::insertBlockAfter(VPExitBlock, MiddleVPBB);
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
@@ -960,15 +967,13 @@ void VPlan::prepareToExecute(Value *TripCountV, Value *VectorTripCountV,
/// have a single predecessor, which is rewired to the new VPIRBasicBlock. All
/// successors of VPBB, if any, are rewired to the new VPIRBasicBlock.
static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) {
- VPIRBasicBlock *IRVPBB = VPIRBasicBlock::fromBasicBlock(IRBB);
+ VPIRBasicBlock *IRVPBB = VPBB->getPlan()->createVPIRBasicBlock(IRBB);
for (auto &R : make_early_inc_range(*VPBB)) {
assert(!R.isPhi() && "Tried to move phi recipe to end of block");
R.moveBefore(*IRVPBB, IRVPBB->end());
}
VPBlockUtils::reassociateBlocks(VPBB, IRVPBB);
-
- delete VPBB;
}
/// Generate the code inside the preheader and body of the vectorized loop.
@@ -1222,6 +1227,7 @@ static void remapOperands(VPBlockBase *Entry, VPBlockBase *NewEntry,
}
VPlan *VPlan::duplicate() {
+ unsigned CreatedBlockSize = CreatedBlocks.size();
// Clone blocks.
const auto &[NewEntry, __] = cloneFrom(Entry);
@@ -1262,9 +1268,23 @@ VPlan *VPlan::duplicate() {
assert(Old2NewVPValues.contains(TripCount) &&
"TripCount must have been added to Old2NewVPValues");
NewPlan->TripCount = Old2NewVPValues[TripCount];
+
+ for (unsigned I = CreatedBlockSize; I != CreatedBlocks.size(); ++I)
+ NewPlan->CreatedBlocks.push_back(CreatedBlocks[I]);
+ CreatedBlocks.truncate(CreatedBlockSize);
+
return NewPlan;
}
+VPIRBasicBlock *VPlan::createVPIRBasicBlock(BasicBlock *IRBB) {
+ auto *VPIRBB = new VPIRBasicBlock(IRBB);
+ for (Instruction &I :
+ make_range(IRBB->begin(), IRBB->getTerminator()->getIterator()))
+ VPIRBB->appendRecipe(new VPIRInstruction(I));
+ CreatedBlocks.push_back(VPIRBB);
+ return VPIRBB;
+}
+
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Twine VPlanPrinter::getUID(const VPBlockBase *Block) {
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 6486c6745a6800..9193c9e873ff3c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -636,9 +636,6 @@ class VPBlockBase {
/// Return the cost of the block.
virtual InstructionCost cost(ElementCount VF, VPCostContext &Ctx) = 0;
- /// Delete all blocks reachable from a given VPBlockBase, inclusive.
- static void deleteCFG(VPBlockBase *Entry);
-
/// Return true if it is legal to hoist instructions into this block.
bool isLegalToHoistInto() {
// There are currently no constraints that prevent an instruction to be
@@ -3585,12 +3582,7 @@ class VPBasicBlock : public VPBlockBase {
/// Clone the current block and it's recipes, without updating the operands of
/// the cloned recipes.
- VPBasicBlock *clone() override {
- auto *NewBlock = new VPBasicBlock(getName());
- for (VPRecipeBase &R : *this)
- NewBlock->appendRecipe(R.clone());
- return NewBlock;
- }
+ VPBasicBlock *clone() override;
protected:
/// Execute the recipes in the IR basic block \p BB.
@@ -3626,20 +3618,11 @@ class VPIRBasicBlock : public VPBasicBlock {
return V->getVPBlockID() == VPBlockBase::VPIRBasicBlockSC;
}
- /// Create a VPIRBasicBlock from \p IRBB containing VPIRInstructions for all
- /// instructions in \p IRBB, except its terminator which is managed in VPlan.
- static VPIRBasicBlock *fromBasicBlock(BasicBlock *IRBB);
-
/// The method which generates the output IR instructions that correspond to
/// this VPBasicBlock, thereby "executing" the VPlan.
void execute(VPTransformState *State) override;
- VPIRBasicBlock *clone() override {
- auto *NewBlock = new VPIRBasicBlock(IRBB);
- for (VPRecipeBase &R : Recipes)
- NewBlock->appendRecipe(R.clone());
- return NewBlock;
- }
+ VPIRBasicBlock *clone() override;
BasicBlock *getIRBasicBlock() const { return IRBB; }
};
@@ -3679,11 +3662,6 @@ class VPRegionBlock : public VPBlockBase {
IsReplicator(IsReplicator) {}
~VPRegionBlock() override {
- if (Entry) {
- VPValue DummyValue;
- Entry->dropAllReferences(&DummyValue);
- deleteCFG(Entry);
- }
}
/// Method to support type inquiry through isa, cast, and dyn_cast.
@@ -3810,6 +3788,8 @@ class VPlan {
/// been modeled in VPlan directly.
DenseMap<const SCEV *, VPValue *> SCEVToExpansion;
+ SmallVector<VPBlockBase *> CreatedBlocks;
+
public:
/// Construct a VPlan with \p Entry to the plan and with \p ScalarHeader
/// wrapping the original header of the scalar loop.
@@ -4026,6 +4006,32 @@ class VPlan {
/// Clone the current VPlan, update all VPValues of the new VPlan and cloned
/// recipes to refer to the clones, and return it.
VPlan *duplicate();
+
+ VPBasicBlock *createVPBasicBlock(const Twine &Name,
+ VPRecipeBase *Recipe = nullptr) {
+ auto *VPB = new VPBasicBlock(Name, Recipe);
+ CreatedBlocks.push_back(VPB);
+ return VPB;
+ }
+
+ VPRegionBlock *createVPRegionBlock(VPBlockBase *Entry, VPBlockBase *Exiting,
+ const std::string &Name = "",
+ bool IsReplicator = false) {
+ auto *VPB = new VPRegionBlock(Entry, Exiting, Name, IsReplicator);
+ CreatedBlocks.push_back(VPB);
+ return VPB;
+ }
+
+ VPRegionBlock *createVPRegionBlock(const std::string &Name = "",
+ bool IsReplicator = false) {
+ auto *VPB = new VPRegionBlock(Name, IsReplicator);
+ CreatedBlocks.push_back(VPB);
+ return VPB;
+ }
+
+ /// Create a VPIRBasicBlock from \p IRBB containing VPIRInstructions for all
+ /// instructions in \p IRBB, except its terminator which is managed in VPlan.
+ VPIRBasicBlock *createVPIRBasicBlock(BasicBlock *IRBB);
};
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 6e633739fcc3dd..02f4c8d8872d82 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -182,7 +182,7 @@ VPBasicBlock *PlainCFGBuilder::getOrCreateVPBB(BasicBlock *BB) {
// Create new VPBB.
StringRef Name = isHeaderBB(BB, TheLoop) ? "vector.body" : BB->getName();
LLVM_DEBUG(dbgs() << "Creating VPBasicBlock for " << Name << "\n");
- VPBasicBlock *VPBB = new VPBasicBlock(Name);
+ VPBasicBlock *VPBB = Plan.createVPBasicBlock(Name);
BB2VPBB[BB] = VPBB;
// Get or create a region for the loop containing BB.
@@ -204,7 +204,7 @@ VPBasicBlock *PlainCFGBuilder::getOrCreateVPBB(BasicBlock *BB) {
if (LoopOfBB == TheLoop) {
RegionOfVPBB = Plan.getVectorLoopRegion();
} else {
- RegionOfVPBB = new VPRegionBlock(Name.str(), false /*isReplicator*/);
+ RegionOfVPBB = Plan.createVPRegionBlock(Name.str(), false /*isReplicator*/);
RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
}
RegionOfVPBB->setEntry(VPBB);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 43c7a935e2f1bc..d672f591abce98 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -297,8 +297,6 @@ static bool mergeReplicateRegionsIntoSuccessors(VPlan &Plan) {
DeletedRegions.insert(Region1);
}
- for (VPRegionBlock *ToDelete : DeletedRegions)
- delete ToDelete;
return !DeletedRegions.empty();
}
@@ -310,7 +308,8 @@ static VPRegionBlock *createReplicateRegion(VPReplicateRecipe *PredRecipe,
assert(Instr->getParent() && "Predicated instruction not in any basic block");
auto *BlockInMask = PredRecipe->getMask();
auto *BOMRecipe = new VPBranchOnMaskRecipe(BlockInMask);
- auto *Entry = new VPBasicBlock(Twine(RegionName) + ".entry", BOMRecipe);
+ auto *Entry =
+ Plan.createVPBasicBlock(Twine(RegionName) + ".entry", BOMRecipe);
// Replace predicated replicate recipe with a replicate recipe without a
// mask but in the replicate region.
@@ -318,7 +317,8 @@ static VPRegionBlock *createReplicateRegion(VPReplicateRecipe *PredRecipe,
PredRecipe->getUnderlyingInstr(),
make_range(PredRecipe->op_begin(), std::prev(PredRecipe->op_end())),
PredRecipe->isUniform());
- auto *Pred = new VPBasicBlock(Twine(RegionName) + ".if", RecipeWithoutMask);
+ auto *Pred =
+ Plan.createVPBasicBlock(Twine(RegionName) + ".if", RecipeWithoutMask);
VPPredInstPHIRecipe *PHIRecipe = nullptr;
if (PredRecipe->getNumUsers() != 0) {
@@ -328,8 +328,10 @@ static VPRegionBlock *createReplicateRegion(VPReplicateRecipe *PredRecipe,
PHIRecipe->setOperand(0, RecipeWithoutMask);
}
PredRecipe->eraseFromParent();
- auto *Exiting = new VPBasicBlock(Twine(RegionName) + ".continue", PHIRecipe);
- VPRegionBlock *Region = new VPRegionBlock(Entry, Exiting, RegionName, true);
+ auto *Exiting =
+ Plan.createVPBasicBlock(Twine(RegionName) + ".continue", PHIRecipe);
+ VPRegionBlock *Region =
+ Plan.createVPRegionBlock(Entry, Exiting, RegionName, true);
// Note: first set Entry as region entry and then connect successors starting
// from it in order, to propagate the "parent" of each VPBasicBlock.
@@ -396,7 +398,6 @@ static bool mergeBlocksIntoPredecessors(VPlan &Plan) {
VPBlockUtils::disconnectBlocks(VPBB, Succ);
VPBlockUtils::connectBlocks(PredVPBB, Succ);
}
- delete VPBB;
}
return !WorkList.empty();
}
@@ -1904,7 +1905,7 @@ void VPlanTransforms::handleUncountableEarlyExit(
if (OrigLoop->getUniqueExitBlock()) {
VPEarlyExitBlock = cast<VPIRBasicBlock>(MiddleVPBB->getSuccessors()[0]);
} else {
- VPEarlyExitBlock = VPIRBasicBlock::fromBasicBlock(
+ VPEarlyExitBlock = Plan.createVPIRBasicBlock(
!OrigLoop->contains(TrueSucc) ? TrueSucc : FalseSucc);
}
@@ -1914,7 +1915,7 @@ void VPlanTransforms::handleUncountableEarlyExit(
IsEarlyExitTaken =
Builder.createNaryOp(VPInstruction::AnyOf, {EarlyExitTakenCond});
- VPBasicBlock *NewMiddle = new VPBasicBlock("middle.split");
+ VPBasicBlock *NewMiddle = Plan.createVPBasicBlock("middle.split");
VPBlockUtils::insertOnEdge(LoopRegion, MiddleVPBB, NewMiddle);
VPBlockUtils::connectBlocks(NewMiddle, VPEarlyExitBlock);
NewMiddle->swapSuccessors();
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -1262,9 +1268,23 @@ VPlan *VPlan::duplicate() { | |||
| assert(Old2NewVPValues.contains(TripCount) && | |||
| "TripCount must have been added to Old2NewVPValues"); | |||
| NewPlan->TripCount = Old2NewVPValues[TripCount]; | |||
|
|
|||
| for (unsigned I = CreatedBlockSize; I != CreatedBlocks.size(); ++I) | |||
There was a problem hiding this comment.
for (unsigned I : seq<unsigned>(CreatedBlockSize, CreatedBlocks.size()))
This patch changes the way blocks are managed by VPlan. Previously all blocks reachable from entry would be cleaned up when a VPlan is destroyed. With this patch, each VPlan keeps track of blocks created for it in a list and this list is then used to delete all blocks in the list when the VPlan is destroyed. To do so, block creation is funneled through helpers in directly in VPlan. The main advantage of doing so is it simplifies CFG transformations, as those do not have to take care of deleting any blocks, just adjusting the CFG. This helps to simplify llvm#108378 and llvm#106748. This also simplifies handling of 'immutable' blocks a VPlan holds references to, which at the moment only include the scalar header block. Note that the original constructors taking VPBlockBase are retained at the moment for unit tests.
fhahn
force-pushed
the
manage-blocks-in-vplan
branch
from
c32ced1 to
af48fcc
Compare
fhahn
added a commit
that referenced
this pull request
Dec 27, 2024
Update C++ unit tests to use VPlanTestBase to construct initial VPlan, using a constructor that creates the VP blocks directly in the constructor. Split off from and in preparation for #120918.
ayalz
reviewed
Dec 29, 2024
|
|
||
| VPBlockBase::deleteCFG(Entry); | ||
| for (auto *VPB : reverse(CreatedBlocks)) |
There was a problem hiding this comment.
Why/Is reverse needed? Are there any assumptions about the order in which blocks are created and appended to a VPlan?
There was a problem hiding this comment.
It is not needed in the latest version, also merge the loops and inlined the only use of dropAllReferences.
| for (auto *VPB : reverse(CreatedBlocks)) | ||
| VPB->dropAllReferences(&DummyValue); | ||
|
|
||
| for (auto *VPB : reverse(CreatedBlocks)) { |
There was a problem hiding this comment.
Not needed in the latest version, thanks
| VPB->dropAllReferences(&DummyValue); | ||
|
|
||
| for (auto *VPB : reverse(CreatedBlocks)) { | ||
| delete VPB; |
There was a problem hiding this comment.
Can VPB be deleted immediately after dropping all references from its recipes (fusing the two loops together), or need to drop all references from recipes of all blocks first?
There was a problem hiding this comment.
Yep, merged the loops. thanks
| } | ||
|
|
||
| /// Create a VPIRBasicBlock from \p IRBB containing VPIRInstructions for all | ||
| /// instructions in \p IRBB, except its terminator which is managed in VPlan. |
There was a problem hiding this comment.
Suggested change
| /// instructions in \p IRBB, except its terminator which is managed in VPlan. | |
| /// instructions in \p IRBB, except its terminator. |
(the block is managed in VPlan, as explained next, rather than its terminator).
There was a problem hiding this comment.
This part refers to the fact that the terminator is dropped and managed via VPlan successors. Maybe needs clarifying?
There was a problem hiding this comment.
Ah, yes, better then to say something like "managed by the successors of VPlan's block". Although VPlan does employ a terminator recipe when a block has more than a single successor?
| @@ -297,8 +297,6 @@ static bool mergeReplicateRegionsIntoSuccessors(VPlan &Plan) { | |||
| DeletedRegions.insert(Region1); | |||
There was a problem hiding this comment.
Suffice to hold bool Changed = false; and set it here to
Suggested change
| DeletedRegions.insert(Region1); | |
| Changed = true; |
There was a problem hiding this comment.
Left as is for now, as we still need to track regions that have been transformed. Renamed to clarify, thanks
| VPBasicBlock *OldEntry = Plan.getEntry(); | ||
| VPBlockUtils::reassociateBlocks(OldEntry, NewEntry); | ||
| Plan.setEntry(NewEntry); | ||
| delete OldEntry; |
There was a problem hiding this comment.
Perhaps worth leaving behind a note that OldEntry is now dead and will be collected later? Same with other deleted deletes.
| @@ -1217,6 +1222,7 @@ static void remapOperands(VPBlockBase *Entry, VPBlockBase *NewEntry, | |||
| } | |||
|
|
|||
| VPlan *VPlan::duplicate() { | |||
| unsigned CreatedBlockSize = CreatedBlocks.size(); | |||
There was a problem hiding this comment.
Suggested change
| unsigned CreatedBlockSize = CreatedBlocks.size(); | |
| unsigned NumBlocksBeforeCloning = CreatedBlocks.size(); |
| @@ -1257,9 +1263,24 @@ VPlan *VPlan::duplicate() { | |||
| assert(Old2NewVPValues.contains(TripCount) && | |||
| "TripCount must have been added to Old2NewVPValues"); | |||
| NewPlan->TripCount = Old2NewVPValues[TripCount]; | |||
|
|
|||
| // Transfer cloned blocks to new VPlan. | |||
There was a problem hiding this comment.
Suggested change
| // Transfer cloned blocks to new VPlan. | |
| // Transfer all cloned blocks (the second half of all current blocks) from current to new VPlan. |
Comment on lines
1268
to
1269
| for (unsigned I : seq<unsigned>(CreatedBlockSize, CreatedBlocks.size())) | ||
| NewPlan->CreatedBlocks.push_back(CreatedBlocks[I]); |
There was a problem hiding this comment.
Suggested change
| for (unsigned I : seq<unsigned>(CreatedBlockSize, CreatedBlocks.size())) | |
| NewPlan->CreatedBlocks.push_back(CreatedBlocks[I]); | |
| unsigned NumBlocksAfterCloning = CreatedBlocks.size(); | |
| for (unsigned I : seq<unsigned>(NumBlocksBeforeCloning, NumBlocksAfterCloning)) | |
| NewPlan->CreatedBlocks.push_back(this->CreatedBlocks[I]); |
| VPBasicBlock *VPBB3 = new VPBasicBlock("VPBB3"); | ||
| VPBasicBlock *VPBB4 = new VPBasicBlock("VPBB4"); | ||
| VPRegionBlock *R1 = new VPRegionBlock(VPBB1, VPBB4); | ||
| VPBasicBlock *VPBB1 = Plan.createVPBasicBlock("VPBB1"); |
There was a problem hiding this comment.
Note that the original constructors taking VPBlockBase are retained at the moment for unit tests.
Which unit tests still need access to original constructors? Better move the latter from 'public' asap, and until then add a comment that VPBB's should be created (and registered) via VPlan::create*() methods, and be destroyed implicitly by VPlan's destructor, rather than be constructed and deleted explicitly.
There was a problem hiding this comment.
I was planning to move them as private separately, thanks.
ayalz
reviewed
Dec 29, 2024
| @@ -2477,7 +2477,7 @@ static void introduceCheckBlockInVPlan(VPlan &Plan, BasicBlock *CheckIRBB) { | |||
| assert(PreVectorPH->getNumSuccessors() == 2 && "Expected 2 successors"); | |||
| assert(PreVectorPH->getSuccessors()[0] == ScalarPH && | |||
| "Unexpected successor"); | |||
| VPIRBasicBlock *CheckVPIRBB = VPIRBasicBlock::fromBasicBlock(CheckIRBB); | |||
| VPIRBasicBlock *CheckVPIRBB = Plan.createVPIRBasicBlock(CheckIRBB); | |||
There was a problem hiding this comment.
This thought can be pursued as follow-up.
A builder can be initialized with VPlan. The builder would provide an API for creating blocks (including but not limited to), whose implementation registers the new block within VPlan. Would be good to create VPlan components consistently, rather than having the abovementioned four distinct mechanisms.
| } | ||
| for (auto *VPB : CreatedBlocks) { | ||
| if (auto *VPBB = dyn_cast<VPBasicBlock>(VPB)) { | ||
| // Replace all operands of recipes and all VPValues define in VPBB with |
There was a problem hiding this comment.
Suggested change
| // Replace all operands of recipes and all VPValues define in VPBB with | |
| // Replace all operands of recipes and all VPValues defined in VPBB with |
| @@ -960,15 +953,13 @@ void VPlan::prepareToExecute(Value *TripCountV, Value *VectorTripCountV, | |||
| /// have a single predecessor, which is rewired to the new VPIRBasicBlock. All | |||
| /// successors of VPBB, if any, are rewired to the new VPIRBasicBlock. | |||
| static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) { | |||
| VPIRBasicBlock *IRVPBB = VPIRBasicBlock::fromBasicBlock(IRBB); | |||
| VPIRBasicBlock *IRVPBB = VPBB->getPlan()->createVPIRBasicBlock(IRBB); | |||
| for (auto &R : make_early_inc_range(*VPBB)) { | |||
| assert(!R.isPhi() && "Tried to move phi recipe to end of block"); | |||
| R.moveBefore(*IRVPBB, IRVPBB->end()); | |||
| } | |||
|
|
|||
| VPBlockUtils::reassociateBlocks(VPBB, IRVPBB); | |||
There was a problem hiding this comment.
Suggested change
| VPBlockUtils::reassociateBlocks(VPBB, IRVPBB); | |
| VPBlockUtils::reassociateBlocks(VPBB, IRVPBB); | |
| // VPBB is now dead and will be cleaned up when the plan gets destroyed. |
| } | ||
|
|
||
| /// Create a VPIRBasicBlock from \p IRBB containing VPIRInstructions for all | ||
| /// instructions in \p IRBB, except its terminator which is managed in VPlan. |
There was a problem hiding this comment.
Ah, yes, better then to say something like "managed by the successors of VPlan's block". Although VPlan does employ a terminator recipe when a block has more than a single successor?
| @@ -396,7 +398,6 @@ static bool mergeBlocksIntoPredecessors(VPlan &Plan) { | |||
| VPBlockUtils::disconnectBlocks(VPBB, Succ); | |||
| VPBlockUtils::connectBlocks(PredVPBB, Succ); | |||
| } | |||
There was a problem hiding this comment.
Suggested change
| } | |
| } | |
| // VPBB is now dead and will be cleaned up when the plan gets destroyed. |
sunshaoce
pushed a commit
to sunshaoce/llvm-public
that referenced
this pull request
Dec 30, 2024
Update C++ unit tests to use VPlanTestBase to construct initial VPlan, using a constructor that creates the VP blocks directly in the constructor. Split off from and in preparation for llvm#120918.
ayalz
reviewed
Dec 30, 2024
| R.setOperand(I, &DummyValue); | ||
| } | ||
| } | ||
| delete VPB; | ||
| } | ||
| for (VPValue *VPV : VPLiveInsToFree) |
There was a problem hiding this comment.
nit: CreatedBlocks >> VPBlocksToFree would be more consistent with VPLiveInsToFree.
ayalz
reviewed
Dec 30, 2024
Comment on lines
+3784
to
+3787
| /// Blocks allocated and owned by the VPlan. They will be deleted once the | ||
| /// VPlan is destroyed. | ||
| SmallVector<VPBlockBase *> CreatedBlocks; | ||
|
|
There was a problem hiding this comment.
nit: perhaps better positioned next to VPLiveInsToFree above.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/4867 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch changes the way blocks are managed by VPlan. Previously all blocks reachable from entry would be cleaned up when a VPlan is destroyed. With this patch, each VPlan keeps track of blocks created for it in a list and this list is then used to delete all blocks in the list when the VPlan is destroyed. To do so, block creation is funneled through helpers in directly in VPlan.
The main advantage of doing so is it simplifies CFG transformations, as those do not have to take care of deleting any blocks, just adjusting the CFG. This helps to simplify
#108378 and #106748.
This also simplifies handling of 'immutable' blocks a VPlan holds references to, which at the moment only include the scalar header block.
Note that the original constructors taking VPBlockBase are retained at the moment for unit tests.