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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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");
----------------
Ayal wrote:
> 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.
Will do separately, thanks!


================
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
----------------
Ayal wrote:
> 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.
Clarified, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:180
+  bool RegionExists = Loop2Region.contains(LoopOfBB);
+  assert(LoopOfBB == TheLoop ||
+         (RegionExists ^ isHeaderBB(BB, LoopOfBB)) &&
----------------
Ayal wrote:
> 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).
Handled the case hat BB is the header of TheLoop in the early exit above, as setEntry in the caller will take care of setting the region. This simplifies the asserts below.


================
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]);
----------------
Ayal wrote:
> Better to assert !isHeaderBB(BB, LoopOfBB) || LoopOfBB == TheLoop here?
Added assert, thanks!


================
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
----------------
Ayal wrote:
> Better to assert isHeaderBB(BB, LoopOfBB) here?
added assert, thanks!


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


================
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:
> Ayal wrote:
> > 
> Note that header/entry and latch/exiting VPBB's of top-level region have yet to be created.
Added the note, thanks!


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


================
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");
----------------
Ayal wrote:
> // Except for the top region, whose predecessor was set when creating VPlan's skeleton.
Clarified, thanks!


================
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)
----------------
Ayal wrote:
> // Except for the top region, whose successor was set when creating VPlan's skeleton.
Clarified, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158333