-
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 4 commits
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9321,14 +9321,17 @@ 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); | ||||||||||||||
|
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. The above explanation regarding "Create initial VPlan skeleton, having ..." remains intact, this change only affects how to get there?
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 think so. |
||||||||||||||
| // Build hierarchical CFG. | ||||||||||||||
| VPlanHCFGBuilder HCFGBuilder(OrigLoop, LI, *Plan); | ||||||||||||||
| // TODO: Convert to VPlan-transform and consoliate all transforms for VPlan | ||||||||||||||
| // creation. | ||||||||||||||
|
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. nit: better keep the comments together?
Suggested change
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! |
||||||||||||||
| HCFGBuilder.buildHierarchicalCFG(); | ||||||||||||||
|
|
||||||||||||||
| 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. | ||||||||||||||
|
|
@@ -9624,13 +9627,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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3537,21 +3537,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); | ||||||||||
|
|
@@ -3617,7 +3602,15 @@ class VPlan { | |||||||||
| /// the original trip count have been replaced. | ||||||||||
| void resetTripCount(VPValue *NewTripCount) { | ||||||||||
| assert(TripCount && NewTripCount && TripCount->getNumUsers() == 0 && | ||||||||||
| "TripCount always must be set"); | ||||||||||
| "TripCount must be set when resetting"); | ||||||||||
| 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 |
||||||||||
| } | ||||||||||
|
|
||||||||||
| // Set the trip count assuming it is currently null; if it is not - use | ||||||||||
| // resetTripCount(). | ||||||||||
|
||||||||||
| // Set the trip count assuming it is currently null; if it is not - use | |
| // resetTripCount(). | |
| /// Set the trip count assuming it is currently null; if it is not - use | |
| /// resetTripCount(). |
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.
Slightly more reasonable to place setTripCount() right after getTripCount() and before resetTripCount()?
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.
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,98 @@ | ||||||||||||
| //===-- VPlanConstruction.cpp - Transforms for initial VPlan construction -===// | ||||||||||||
| // | ||||||||||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||||||||||||
| // See https://llvm.org/LICENSE.txt for license information. | ||||||||||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||||||||||||
| // | ||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||
| /// | ||||||||||||
| /// \file | ||||||||||||
| /// This file implements transforms for initial VPlan construction | ||||||||||||
| /// | ||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||
|
|
||||||||||||
| #include "LoopVectorizationPlanner.h" | ||||||||||||
| #include "VPlan.h" | ||||||||||||
| #include "VPlanCFG.h" | ||||||||||||
| #include "VPlanTransforms.h" | ||||||||||||
| #include "llvm/Analysis/LoopInfo.h" | ||||||||||||
| #include "llvm/Analysis/ScalarEvolution.h" | ||||||||||||
|
|
||||||||||||
| using namespace llvm; | ||||||||||||
|
|
||||||||||||
| void VPlanTransforms::introduceTopLevelVectorLoopRegion( | ||||||||||||
| VPlan &Plan, Type *InductionTy, PredicatedScalarEvolution &PSE, | ||||||||||||
| bool RequiresScalarEpilogueCheck, bool TailFolded, Loop *TheLoop) { | ||||||||||||
| // TODO: Generalize to introduce all loop regions. | ||||||||||||
| 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); | ||||||||||||
| assert(OriginalLatch->getNumSuccessors() == 0 && "expected no predecessors"); | ||||||||||||
|
||||||||||||
| assert(OriginalLatch->getNumSuccessors() == 0 && "expected no predecessors"); | |
| assert(OriginalLatch->getNumSuccessors() == 0 && "VPlan should end at top level latch"); |
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.
| // filled | |
| // 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.
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.
Would be good to justify setting parenthood by explaining something like
| for (VPBlockBase *VPBB : vp_depth_first_shallow(HeaderVPBB)) | |
| VPBB->setParent(TopRegion); | |
| // All VPBB's reachable shallowly from HeaderVPBB belong to top level loop, because VPlan is expected to end at top level latch. | |
| for (VPBlockBase *VPBB : vp_depth_first_shallow(HeaderVPBB)) | |
| VPBB->setParent(TopRegion); |
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.
Should files designed to hold VPlan tranforms be called
VPlanTransforms*.cppto help classify them in the absence of a subdirectory, as in VPlanTransformUnroll, VPlanTransformPredicate, etc.,?What transform(s) are destines to reside in this file - all the preliminary Scalar Transforms according to roadmap (
VPlanTransformsScalar.cpp?), or only some of them?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.
Sounds good! We could also introduce a subfolder?
In the short term, in addition also the code to build the initial VPlan (i.e. what currently resides in VPHCFGBuilder) and possibly recipe widening?
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.
Ok, let's go with VPlanConstruction.cpp for now, which expresses what it's about rather than how (may or may not involve VPlanTransforms).