[PATCH] D158333: [VPlan] Move initial skeleton construction to createInitialVPlan. (NFC)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In summary: "This patch moves creating **the preheader and** middle VPBBs as well as the
initial empty vector loop region to createInitialVPlan" - only middle VPBB and region are moved, preheader is already there.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:713
 VPlanPtr VPlan::createInitialVPlan(const SCEV *TripCount, ScalarEvolution &SE) {
   VPBasicBlock *Preheader = new VPBasicBlock("ph");
   VPBasicBlock *VecPreheader = new VPBasicBlock("vector.ph");
----------------
This should be `auto Entry = new VPBasicBlock("entry")` plus updating VPlan's "entry" to refer to this VPBB rather than VecPreHeader. Can be done in a separate patch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2553
 
-  /// Create an initial VPlan with preheader and entry blocks. Creates a
-  /// VPExpandSCEVRecipe for \p TripCount and uses it as plan's trip count.
+  /// Create initial VPlan skeleton, having a VPBasicBlock for the pre-header
+  /// which contains SCEV expansions that need to happen before the CFG is
----------------
The original scalar pre-header no longer acts as a pre-header in VPlan's skeleton and best be renamed "entry". Can be done as part of rewording VPlan's "Entry" and "VectorPreHeader" in a separate patch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:180
+  bool RegionExists = Loop2Region.contains(LoopOfBB);
+  assert(LoopOfBB == TheLoop ||
+         (RegionExists ^ isHeaderBB(BB, LoopOfBB)) &&
----------------
If LoopOfBB == TheLoop then RegionExists because it was constructed in buildInitialVPlan, but the header VPBB of TheLoop is not - it gets constructed by calling getOrCreateVPBB(). I.e., that BB is exceptional in being a header (not in BB2VPBB) of an existing region (in Loop2Region).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:183
+             "region must exist or BB must be a loop header");
+  if (RegionExists) {
+    VPBB->setParent(Loop2Region[LoopOfBB]);
----------------
Better to assert !isHeaderBB(BB, LoopOfBB) || LoopOfBB == TheLoop here?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:184
+  if (RegionExists) {
+    VPBB->setParent(Loop2Region[LoopOfBB]);
   } else {
----------------
(otherwise VPBB's parent gets set to a new region upon constructing the latter below.)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:185
+    VPBB->setParent(Loop2Region[LoopOfBB]);
   } else {
     // If BB's loop is nested inside another loop within VPlan's scope, the
----------------
Better to assert isHeaderBB(BB, LoopOfBB) here?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:191
     auto *RegionOfVPBB = new VPRegionBlock(
         LoopOfBB->getHeader()->getName().str(), false /*isReplicator*/);
     RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:320
+  // skeleton. These were created directly rather than via getOrCreateVPBB(),
+  // revisit them now to update BB2VPBB and Loop2region
+  VPRegionBlock *TheRegion = Plan.getVectorLoopRegion();
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:320
+  // skeleton. These were created directly rather than via getOrCreateVPBB(),
+  // revisit them now to update BB2VPBB and Loop2region
+  VPRegionBlock *TheRegion = Plan.getVectorLoopRegion();
----------------
Ayal wrote:
> 
Note that header/entry and latch/exiting VPBB's of top-level region have yet to be created.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:367-369
     if (!isHeaderBB(BB, LoopForBB))
       setVPBBPredsFromBB(VPBB, BB);
     else {
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:370
     else {
       // BB is a loop header, set the predecessor for the region.
       assert(isHeaderVPBB(VPBB) && "isHeaderBB and isHeaderVPBB disagree");
----------------
// Except for the top region, whose predecessor was set when creating VPlan's skeleton.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:401
     // For a latch we need to set the successor of the region rather than that
     // of VPBB and it should be set to the exit, i.e., non-header successor.
+    if (TheRegion != Region)
----------------
// Except for the top region, whose successor was set when creating VPlan's skeleton.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:337
+  HeaderVPBB->setName("vector.body");
+  HeaderVPBB->setParent(TheRegion);
+  TheRegion->setEntry(HeaderVPBB);
----------------
fhahn wrote:
> Ayal wrote:
> > Is setting the parent of HeaderVPBB to TheRegion redundant - getOrCreateVPBB() should have done so already?
> Indeed, removed, thanks!
OTOH, perhaps its clearer to create HeaderVPBB next to stage 0 above:
```
  BasicBlock *HeaderBB = TheLoop->getHeader();
  VPBasicBlock *HeaderVPBB = new VPBasicBlock(HeaderBB->getName());
  BB2VPBB[HeaderBB] = HeaderVPBB;
  HeaderVPBB->setParent(TheRegion);
```
thereby also slightly simplifying getOrCreateVPBB?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158333/new/

https://reviews.llvm.org/D158333