-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[VPlan] Refactor VPlan creation, add transform introducing region (NFC). #128419
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 1 commit
4a3ec74
0be6939
99ad49e
f605097
bc00209
64751d2
170bc6c
e5ef6d3
14ce627
d7a023a
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 |
|---|---|---|
|
|
@@ -9312,14 +9312,15 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) { | |
| return !CM.requiresScalarEpilogue(VF.isVector()); | ||
| }, | ||
| Range); | ||
| VPlanPtr Plan = VPlan::createInitialVPlan(Legal->getWidestInductionType(), | ||
| PSE, RequiresScalarEpilogueCheck, | ||
| CM.foldTailByMasking(), OrigLoop); | ||
|
|
||
| auto Plan = std::make_unique<VPlan>(OrigLoop); | ||
| // Build hierarchical CFG. | ||
| VPlanHCFGBuilder HCFGBuilder(OrigLoop, LI, *Plan); | ||
| HCFGBuilder.buildHierarchicalCFG(); | ||
|
Collaborator
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. Looks like buildHierarchicalCFG should now (as in TODO) also be a VPlanTransform.
Contributor
Author
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. Yep that sounds good. I think it would make sense to consolidate the transforms for initial VPlan construction into a separate |
||
|
|
||
| VPlanTransforms::introduceTopLevelVectorLoopRegion( | ||
|
Collaborator
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. Should this introduce all regions, i.e., lifting a flat CFG into a hierarchical one? With the inverse lowing conversion taking place at the end, to simplify code-gen.
Contributor
Author
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. Yep, I added a TODO for a follow-up. As this is for now only needed in the native path, it's probably best to do it separately. |
||
| *Plan, Legal->getWidestInductionType(), PSE, RequiresScalarEpilogueCheck, | ||
| CM.foldTailByMasking(), OrigLoop); | ||
|
|
||
| // Don't use getDecisionAndClampRange here, because we don't know the UF | ||
| // so this function is better to be conservative, rather than to split | ||
| // it up into different VPlans. | ||
|
|
@@ -9615,13 +9616,14 @@ VPlanPtr LoopVectorizationPlanner::buildVPlan(VFRange &Range) { | |
| assert(EnableVPlanNativePath && "VPlan-native path is not enabled."); | ||
|
|
||
| // Create new empty VPlan | ||
| auto Plan = VPlan::createInitialVPlan(Legal->getWidestInductionType(), PSE, | ||
| true, false, OrigLoop); | ||
|
|
||
| auto Plan = std::make_unique<VPlan>(OrigLoop); | ||
| // Build hierarchical CFG | ||
| VPlanHCFGBuilder HCFGBuilder(OrigLoop, LI, *Plan); | ||
| HCFGBuilder.buildHierarchicalCFG(); | ||
|
|
||
| VPlanTransforms::introduceTopLevelVectorLoopRegion( | ||
| *Plan, Legal->getWidestInductionType(), PSE, true, false, OrigLoop); | ||
|
|
||
| for (ElementCount VF : Range) | ||
| Plan->addVF(VF); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3505,21 +3505,6 @@ class VPlan { | |||||||||||||
| VPBB->setPlan(this); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// Create initial VPlan, having an "entry" VPBasicBlock (wrapping | ||||||||||||||
| /// original scalar pre-header) which contains SCEV expansions that need | ||||||||||||||
| /// to happen before the CFG is modified (when executing a VPlan for the | ||||||||||||||
| /// epilogue vector loop, the original entry needs to be replaced by a new | ||||||||||||||
| /// one); a VPBasicBlock for the vector pre-header, followed by a region for | ||||||||||||||
| /// the vector loop, followed by the middle VPBasicBlock. If a check is needed | ||||||||||||||
| /// to guard executing the scalar epilogue loop, it will be added to the | ||||||||||||||
| /// middle block, together with VPBasicBlocks for the scalar preheader and | ||||||||||||||
| /// exit blocks. \p InductionTy is the type of the canonical induction and | ||||||||||||||
| /// used for related values, like the trip count expression. | ||||||||||||||
| static VPlanPtr createInitialVPlan(Type *InductionTy, | ||||||||||||||
| PredicatedScalarEvolution &PSE, | ||||||||||||||
| bool RequiresScalarEpilogueCheck, | ||||||||||||||
| bool TailFolded, Loop *TheLoop); | ||||||||||||||
|
|
||||||||||||||
| /// Prepare the plan for execution, setting up the required live-in values. | ||||||||||||||
| void prepareToExecute(Value *TripCount, Value *VectorTripCount, | ||||||||||||||
| VPTransformState &State); | ||||||||||||||
|
|
@@ -3589,6 +3574,8 @@ class VPlan { | |||||||||||||
| TripCount = NewTripCount; | ||||||||||||||
|
Collaborator
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. Above error message that "TripCount always must be set" needs to be updated.
Contributor
Author
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. Done, thanks |
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| void setTripCount(VPValue *NewTripCount) { TripCount = NewTripCount; } | ||||||||||||||
|
||||||||||||||
| void setTripCount(VPValue *NewTripCount) { TripCount = NewTripCount; } | |
| // Set the trip count assuming it is currently null; if it is not - use resetTripCount(). | |
| void setTripCount(VPValue *NewTripCount) { | |
| assert(!TripCount && "TripCount expected to be null"); | |
| TripCount = NewTripCount; | |
| } |
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.
Done, thanks
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -180,7 +180,7 @@ VPBasicBlock *PlainCFGBuilder::getOrCreateVPBB(BasicBlock *BB) { | |||||||
|
|
||||||||
| // Get or create a region for the loop containing BB. | ||||||||
|
||||||||
| // Get or create a region for the loop containing BB. | |
| // Get or create a region for the loop containing BB, except for the top region of TheLoop which is created later. |
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.
Done 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.
Drop the if (LoopOfBB == TheLoop) { case below which is now dead.
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.
Done 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.
TheRegion and VectorPreheaderVPBB are yet to be formed, hence dropping their recording in BB2VPBB and Loop2Region?
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, will be introduced as part of the transform.
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.
(btw, LI->getLoopFor(TheLoop->getHeader()) is aka TheLoop?)
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
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.
The header of TheLoop need not be mapped in BB2VPBB (yet)? Its corresponding VPBB cannot be retrieved via TheRegion (yet).
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, there's no region yet for TheLoop, only the initial entry block to the VPlan.
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.
Comment needs to be updated - the top region itself has yet to be introduced, rather than its predecessor.
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.
Done, 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.
Fold this if with its preceding else?
Check instead if (Region)?
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.
Done 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.
Formatting.
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.
Should be 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.
| // except for the top region, whose successor was set when creating | |
| // VPlan's skeleton. | |
| // except for the top region, which is handled elsewhere. |
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.
Done 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.
Drop this continue;?
How does this work with the following code being unreachable - is it really needed? tested?
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.
The code was left over from earlier iterations, removed, 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 be folded with handling TheLoop's latch above, with an early continue if LoopForBB == TheLoop.
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.
Code here not needed, removed, thanks
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,82 @@ | |||||
|
|
||||||
| using namespace llvm; | ||||||
|
|
||||||
| void VPlanTransforms::introduceTopLevelVectorLoopRegion( | ||||||
| VPlan &Plan, Type *InductionTy, PredicatedScalarEvolution &PSE, | ||||||
| bool RequiresScalarEpilogueCheck, bool TailFolded, Loop *TheLoop) { | ||||||
| auto *HeaderVPBB = cast<VPBasicBlock>(Plan.getEntry()->getSingleSuccessor()); | ||||||
| VPBlockUtils::disconnectBlocks(Plan.getEntry(), HeaderVPBB); | ||||||
|
|
||||||
| VPBasicBlock *OriginalLatch = | ||||||
| cast<VPBasicBlock>(HeaderVPBB->getSinglePredecessor()); | ||||||
| VPBlockUtils::disconnectBlocks(OriginalLatch, HeaderVPBB); | ||||||
|
||||||
| VPBasicBlock *VecPreheader = Plan.createVPBasicBlock("vector.ph"); | ||||||
| VPBlockUtils::connectBlocks(Plan.getEntry(), VecPreheader); | ||||||
|
|
||||||
| // Create SCEV and VPValue for the trip count. | ||||||
| // We use the symbolic max backedge-taken-count, which works also when | ||||||
| // vectorizing loops with uncountable early exits. | ||||||
| const SCEV *BackedgeTakenCountSCEV = PSE.getSymbolicMaxBackedgeTakenCount(); | ||||||
| assert(!isa<SCEVCouldNotCompute>(BackedgeTakenCountSCEV) && | ||||||
| "Invalid loop count"); | ||||||
| ScalarEvolution &SE = *PSE.getSE(); | ||||||
| const SCEV *TripCount = SE.getTripCountFromExitCount(BackedgeTakenCountSCEV, | ||||||
| InductionTy, TheLoop); | ||||||
| Plan.setTripCount( | ||||||
| vputils::getOrCreateVPValueForSCEVExpr(Plan, TripCount, SE)); | ||||||
|
|
||||||
| // Create VPRegionBlock, with empty header and latch blocks, to be filled | ||||||
|
||||||
| // Create VPRegionBlock, with empty header and latch blocks, to be filled | |
| // Create VPRegionBlock, with existing header and new empty latch block, to be filled |
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.
Updated, 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.
Is a new LatchVPBB needed or can OriginalLatch continue to serve as the region's latch? Initially separate empty header and latch were created here to support subsequent introduction of VPBB's in between.
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.
There are some places where we rely on there to the a separate latch, e.g. when adding canonical IV related recipes. Those could be updated separately.
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.
Use RPOT and break when reaching LatchVPBB?
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.
Left as is for now, as this is just to set the parent, so RPOT should not be needed
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -52,6 +52,21 @@ struct VPlanTransforms { | |||||
| verifyVPlanIsValid(Plan); | ||||||
| } | ||||||
|
|
||||||
| /// Introduce the top-level VPRegionBlock for the main loop in \p Plan. Coming | ||||||
| /// in this function, \p Plan's top-level loop is modeled using a plain CFG. | ||||||
|
||||||
| /// in this function, \p Plan's top-level loop is modeled using a plain CFG. | |
| /// into this function, \p Plan's top-level loop is modeled using a plain CFG. |
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.
Updated, 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.
| /// This transforms replaces the plain CFG with a VPRegionBlock wrapping the | |
| /// This transform wraps the plain CFG of the top-level loop within a VPRegionBlock |
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.
Updated, 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.
| /// top-level loop and creates a VPValue expressions for the original trip | |
| /// and creates a VPValue expressions for the original trip |
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.
Done 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.
The above explanation regarding "Create initial VPlan skeleton, having ..." remains intact, this change only affects how to get there?
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 I think so.