-
Notifications
You must be signed in to change notification settings - Fork 15k
[VPlan] Manage created blocks directly in VPlan. (NFC) #120918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
dd45cad
e72a71f
407dbc1
af48fcc
f51412a
ec4f283
0b80912
fc34ca5
bc7f445
8f83ad8
d6b4d78
99924fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2454,7 +2454,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; | ||
| } | ||
|
|
@@ -8084,11 +8084,11 @@ 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; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps worth leaving behind a note that OldEntry is now dead and will be collected later? Same with other deleted deletes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added, thanks! |
||
| // OldEntry is now dead and will be cleaned up when the plan gets destroyed. | ||
|
|
||
| introduceCheckBlockInVPlan(Plan, Insert); | ||
| return Insert; | ||
|
|
@@ -9289,7 +9289,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) { | |
| VPBB->appendRecipe(Recipe); | ||
| } | ||
|
|
||
| VPBlockUtils::insertBlockAfter(new VPBasicBlock(), VPBB); | ||
| VPBlockUtils::insertBlockAfter(Plan->createVPBasicBlock(""), VPBB); | ||
| VPBB = cast<VPBasicBlock>(VPBB->getSingleSuccessor()); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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,13 @@ void VPIRBasicBlock::execute(VPTransformState *State) { | |||||||||
| connectToPredecessors(State->CFG); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| VPIRBasicBlock *VPIRBasicBlock::clone() { | ||||||||||
| auto *NewBlock = getPlan()->createEmptyVPIRBasicBlock(IRBB); | ||||||||||
| for (VPRecipeBase &R : Recipes) | ||||||||||
| NewBlock->appendRecipe(R.clone()); | ||||||||||
|
Comment on lines
+474
to
+475
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New constructor should avoid need to rename. |
||||||||||
| return NewBlock; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void VPBasicBlock::execute(VPTransformState *State) { | ||||||||||
| bool Replica = bool(State->Lane); | ||||||||||
| BasicBlock *NewBB = State->CFG.PrevBB; // Reuse it if possible. | ||||||||||
|
|
@@ -513,14 +515,11 @@ void VPBasicBlock::execute(VPTransformState *State) { | |||||||||
| executeRecipes(State, NewBB); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void VPBasicBlock::dropAllReferences(VPValue *NewValue) { | ||||||||||
| for (VPRecipeBase &R : Recipes) { | ||||||||||
| for (auto *Def : R.definedValues()) | ||||||||||
| Def->replaceAllUsesWith(NewValue); | ||||||||||
|
|
||||||||||
| for (unsigned I = 0, E = R.getNumOperands(); I != E; I++) | ||||||||||
| R.setOperand(I, 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) { | ||||||||||
|
|
@@ -541,7 +540,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,20 +700,13 @@ 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; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void VPRegionBlock::dropAllReferences(VPValue *NewValue) { | ||||||||||
| for (VPBlockBase *Block : vp_depth_first_shallow(Entry)) | ||||||||||
| // Drop all references in VPBasicBlocks and replace all uses with | ||||||||||
| // DummyValue. | ||||||||||
| Block->dropAllReferences(NewValue); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void VPRegionBlock::execute(VPTransformState *State) { | ||||||||||
| ReversePostOrderTraversal<VPBlockShallowTraversalWrapper<VPBlockBase *>> | ||||||||||
| RPOT(Entry); | ||||||||||
|
|
@@ -822,32 +814,33 @@ 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 : CreatedBlocks) { | ||||||||||
| if (auto *VPBB = dyn_cast<VPBasicBlock>(VPB)) { | ||||||||||
| // Replace all operands of recipes and all VPValues define in VPBB with | ||||||||||
|
||||||||||
| // Replace all operands of recipes and all VPValues define in VPBB with | |
| // Replace all operands of recipes and all VPValues defined in VPBB with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed thanks
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, merged the loops. thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: CreatedBlocks >> VPBlocksToFree would be more consistent with VPLiveInsToFree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| VPBlockUtils::reassociateBlocks(VPBB, IRVPBB); | |
| VPBlockUtils::reassociateBlocks(VPBB, IRVPBB); | |
| // VPBB is now dead and will be cleaned up when the plan gets destroyed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such construction of VPBB's seems complementary of
VPBuilder,VPlanHCFGBuilder,VPRecipeBuilder?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, but in this case it is directly tied to VPlan so we can track liveness conveniently; we could track liveness separately from VPlan, but then we would have 2 data structures to keep in sync and to delete together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.