[VPlan] Simplify Plan's entry in removeBranchOnConst. #154510
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2352,9 +2352,9 @@ EpilogueVectorizerMainLoop::createIterationCountCheck(ElementCount VF, | |||||
| /// VPBB are moved to the end of the newly created VPIRBasicBlock. VPBB must | ||||||
| /// have a single predecessor, which is rewired to the new VPIRBasicBlock. All | ||||||
| /// successors of VPBB, if any, are rewired to the new VPIRBasicBlock. | ||||||
| static VPIRBasicBlock *replaceVPBBWithIRVPBB(VPlan &Plan, VPBasicBlock *VPBB, | ||||||
| BasicBlock *IRBB) { | ||||||
| VPIRBasicBlock *IRVPBB = Plan.createVPIRBasicBlock(IRBB); | ||||||
| auto IP = IRVPBB->begin(); | ||||||
| for (auto &R : make_early_inc_range(VPBB->phis())) | ||||||
| R.moveBefore(*IRVPBB, IP); | ||||||
|
|
@@ -2565,6 +2565,9 @@ void InnerLoopVectorizer::fixVectorizedLoop(VPTransformState &State) { | |||||
| // Remove redundant induction instructions. | ||||||
| cse(HeaderBB); | ||||||
|
|
||||||
| if (Plan.getScalarPreheader()->getNumPredecessors() == 0) | ||||||
|
||||||
| return; | ||||||
|
|
||||||
|
||||||
| // Set/update profile weights for the vector and remainder loops as original | ||||||
| // loop iterations are now distributed among them. Note that original loop | ||||||
| // becomes the scalar remainder loop after vectorization. | ||||||
|
|
@@ -7220,6 +7223,12 @@ DenseMap<const SCEV *, Value *> LoopVectorizationPlanner::executePlan( | |||||
| VPlanTransforms::optimizeForVFAndUF(BestVPlan, BestVF, BestUF, PSE); | ||||||
| VPlanTransforms::simplifyRecipes(BestVPlan); | ||||||
| VPlanTransforms::removeBranchOnConst(BestVPlan); | ||||||
| if (BestVPlan.getEntry()->getSingleSuccessor() == | ||||||
| BestVPlan.getScalarPreheader()) { | ||||||
|
Comment on lines
+7190
to
+7191
Collaborator
There was a problem hiding this comment. removeBranchOnConst() could conceivably bypass the vector loop; this actually happens in few tests. Worth emitting a missed-vectorization remark.
Contributor
Author
There was a problem hiding this comment. Yep, added an analysis remark. I am not sure if missed-vectorization would be accurate, because this is for cases where we would create a dead vector loop and should not even try to vectorize.
Collaborator
There was a problem hiding this comment. ok, it appears the loop isn't vectorized because the Trip Count guard is known to always jump to the scalar loop, i.e., where VFxUF is known to exceed TC, so conceptually a smaller VFxUF could work. But tests include unvectorizable non-loop cases where TC<=1, which should better be cleaned up before calling LV, certainly before reaching LVP::executePlan().
Contributor
Author
There was a problem hiding this comment. Agreed, we already have a TODO where we created the known True condition
Collaborator
There was a problem hiding this comment. We have a TODO here too; wondering if the message should specify that vectorization is dead or never executes - due to insufficient trip-count.
Contributor
Author
There was a problem hiding this comment. Updated message to mention insufficient trip count, thanks |
||||||
| // TODO: Should not even try to vectorize. | ||||||
| return DenseMap<const SCEV *, Value *>(); | ||||||
| } | ||||||
|
|
||||||
| VPlanTransforms::narrowInterleaveGroups( | ||||||
| BestVPlan, BestVF, | ||||||
| TTI.getRegisterBitWidth(TargetTransformInfo::RGK_FixedWidthVector)); | ||||||
|
|
@@ -7262,7 +7271,7 @@ DenseMap<const SCEV *, Value *> LoopVectorizationPlanner::executePlan( | |||||
| BasicBlock *EntryBB = | ||||||
| cast<VPIRBasicBlock>(BestVPlan.getEntry())->getIRBasicBlock(); | ||||||
| State.CFG.PrevBB = ILV.createVectorizedLoopSkeleton(); | ||||||
| replaceVPBBWithIRVPBB(BestVPlan, BestVPlan.getScalarPreheader(), | ||||||
|
||||||
| State.CFG.PrevBB->getSingleSuccessor()); | ||||||
| VPlanTransforms::removeDeadRecipes(BestVPlan); | ||||||
|
|
||||||
|
|
@@ -7345,8 +7354,9 @@ DenseMap<const SCEV *, Value *> LoopVectorizationPlanner::executePlan( | |||||
| } else { | ||||||
| // Keep all loop hints from the original loop on the vector loop (we'll | ||||||
| // replace the vectorizer-specific hints below). | ||||||
| if (BestVPlan.getScalarPreheader()->getNumPredecessors() > 0) | ||||||
|
||||||
| if (BestVPlan.getScalarPreheader()->getNumPredecessors() > 0) | |
| } else if (BestVPlan.getScalarPreheader()->getNumPredecessors() > 0) { |
There was a problem hiding this comment.
The code below will execute in in the general else{, so I left it as-is for now.
Outdated
There was a problem hiding this comment.
"Native" VPlan constructs can simply be discarded when they become dead or unreached (even then lazily), whereas VPlan constructs that model the scalar loop are kept orphaned after becoming unreachable, to be processed here.
This raises an old thought: VPlan specifies code to be generated, using its blocks and recipes; how/could VPlan be extended to also dismantle existing code, perhaps using "anti-recipes" or "anti-blocks" whose execute() performs the desired clean-up.
There was a problem hiding this comment.
Yep, unfortunately we cannot do this when the VPIRBasicBlock for the scalar preheader gets executed, as we only execute reachable VPBBs.
I moved it for now to VPlan::execute. Unfortunately doing in the destructor of VPIRBasicBlock won't work either, because we need access to LoopInfo.
There was a problem hiding this comment.
Having it in VPlan::execute is great!
As a potential follow-up: the VPIRBasicBlock of the scalar preheader and/or a VPIRRegionBlock of the scalar loop could be marked for destruction in VPlan if unreachable (as in CreatedBlocks), indicating that its loop should be removed from LoopInfo etc.
There was a problem hiding this comment.
Yep, they are implicitly marked for destruction (all VPIRBasicBlocks that are unreachable are tracked in CreatedBlocks) and can be destroyed in the destructor; but they need LoopInfo/DT passed somehow, to notify those about the removal
Outdated
There was a problem hiding this comment.
This is recorded here instead of asking if BestPlan reaches the scalar loop below (next to early exiting if it doesn't), because it's relevant when vectorizing the main loop only? When also vectorizing the epilog loop, should the we check if BestEpiPlan reaches the scalar loop instead?
There was a problem hiding this comment.
It is only relevant when not vectorizing the epilogue for now, as when vectorizing the epilogue we don't materilize the check yet.
The flag is set here to still keep the assert below before exiting. It could be moved/duplicated, but eventually we also will also enable it for the epilogue vectorization case
Outdated
There was a problem hiding this comment.
Early-exit worth a comment - skip the end which updates the scalar loop if it's removed?
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 change is needed because VPBB may no longer be reachable from Plan's entry. VPBB must still have a single predecessor, as documented above, but that pred might be pred free? If only some callers pass an unreachable VPBB, Plan could be an optional parameter, to keep other callers intact.
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, updated the comment to say that all predecessors/successors are rewired, if any and added Plan as optional parameter, which must be passed if the may be unreachable, thanks!