[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/LoopVectorize.cpp:8882
   // the vector loop, followed by the middle basic block. The skeleton vector
   // loop region contains a header and latch basic blocks.
   VPlanPtr Plan = VPlan::createInitialVPlan(
----------------
Ayal wrote:
> Moving the creation of top region and middle block into createInitialVPlan looks good, and follows the description above, except for the last sentence. Documentation of createInitialVPlan() in VPlan.h needs updating to keep in sync.
> The initial VPlan conceptually consists of pre-header, top-region, and exit, all representing existing IR "in-place". A vector-pre-header is also part if the initial VPlan, currently.
> 
> The last sentence above: filling the top-region with header, latch, followed by VPBB's in-between, should take place as independent steps (VPlan-to-VPlan transforms), potentially after caching a cloned copy of the original "in-place" top-region to be used for fallback and/or remainder loops. Possibly as follow-up patches.
> Moving the creation of top region and middle block into createInitialVPlan looks good, and follows the description above, except for the last sentence. Documentation of createInitialVPlan() in VPlan.h needs updating to keep in sync.
The initial VPlan conceptually consists of pre-header, top-region, and exit, all representing existing IR "in-place". A vector-pre-header is also part if the initial VPlan, currently.

Updated the comment in VPlan.h to be in sync with the statement here (except last sentence).

> The last sentence above: filling the top-region with header, latch, followed by VPBB's in-between, should take place as independent steps (VPlan-to-VPlan transforms), potentially after caching a cloned copy of the original "in-place" top-region to be used for fallback and/or remainder loops. Possibly as follow-up patches.

Creating the header & latch VPBBs is mostly done up-front at the moment, to simplify the code for recipe creation we have now. Going forward, it should probably be possible to eventually just use the blocks created during initial VPlan construction (like in the native path currently) and what's left to do is adding the canonical induction recipes separately.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8892
+  Plan->getVectorLoopRegion()->setExiting(LatchVPBB);
 
   // so this function is better to be conservative, rather than to split
----------------
Ayal wrote:
> Comment line dropped accidentally? BTW, not sure I follow why splitting the range is avoided here nor how taking a uniform conservative decision across all VF's in Range relates to UF being unknown(?).
Ah yes, accidentally remove. Added the comment back, thanks!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8997
   // Adjust the recipes for any inloop reductions.
-  adjustRecipesForReductions(cast<VPBasicBlock>(TopRegion->getExiting()), Plan,
-                             RecipeBuilder, Range.Start);
+  adjustRecipesForReductions(LatchVPBB, Plan, RecipeBuilder, Range.Start);
 
----------------
Ayal wrote:
> nit: independent cleanup?
Split off to b1115f8ccefb380824a9d997622cc84fc0d84a89


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:723
+  auto *TopRegion = new VPRegionBlock("vector loop", false /*isReplicator*/);
+  VPBlockUtils::insertBlockAfter(TopRegion, Plan->getEntry());
+  VPBasicBlock *MiddleVPBB = new VPBasicBlock("middle.block");
----------------
Ayal wrote:
> 
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:721
       vputils::getOrCreateVPValueForSCEVExpr(*Plan, TripCount, SE);
+  auto *TopRegion = new VPRegionBlock("vector loop", false /*isReplicator*/);
+  VPBlockUtils::insertBlockAfter(TopRegion, Plan->getEntry());
----------------
Ayal wrote:
> nit: worth a comment that the top region is initially constructed empty here, as a placeholder, to be filled later with VPBB's for subsequent processing.
Added a comment, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2544
+  /// which contains SCEV expansions that need to happen before the CFG is
+  /// modified; a VPbasicBlock for the vector pre-header, followed by a region
+  /// for the vector loop, followed by the middle VPBasicBlock.
----------------
Ayal wrote:
> 
Fixed, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:307
+  // 0. Reuse the top-level region, vector-preheader and exit VPBBs from the
+  // skeleton.
+  VPRegionBlock *TheRegion = Plan.getVectorLoopRegion();
----------------
Ayal wrote:
> // These were created directly rather than via getOrCreateVPBB(), revisit them now to update BB2VPBB and Loop2region.
Added, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:313
+         "Unexpected loop preheader");
+  VPBasicBlock *VectorPreheaderVPBB = Plan.getEntry();
+  BB2VPBB[ThePreheaderBB] = VectorPreheaderVPBB;
----------------
Ayal wrote:
> nit:
> `  auto *VectorPreheaderVPBB = cast<VPBasicBlock>(TheRegion->getSinglePredecessor());`
> would be more consistent with how middle VPBB is retrieved.
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:314
+  VPBasicBlock *VectorPreheaderVPBB = Plan.getEntry();
+  BB2VPBB[ThePreheaderBB] = VectorPreheaderVPBB;
+  BasicBlock *LoopExitBB = TheLoop->getUniqueExitBlock();
----------------
Ayal wrote:
> Worth noting that `ThePreheaderBB` conceptually corresponds to both Plan.getPreheader() (which wraps the original preheader BB) and Plan.getEntry() (which represents the new vector preheader); here we're interested in setting BB2VPBB to the latter.
added, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:328
   // LoopBlocksDFS.
-  BasicBlock *ThePreheaderBB = TheLoop->getLoopPreheader();
-  assert((ThePreheaderBB->getTerminator()->getNumSuccessors() == 1) &&
-         "Unexpected loop preheader");
-  VPBasicBlock *ThePreheaderVPBB = Plan.getEntry();
-  BB2VPBB[ThePreheaderBB] = ThePreheaderVPBB;
-  ThePreheaderVPBB->setName("vector.ph");
+  VectorPreheaderVPBB->setName("vector.ph");
   for (auto &I : *ThePreheaderBB) {
----------------
Ayal wrote:
> Is this redundant - createInitialVPlan() already sets the name of VectorPreheaderVPBB to "vector.ph" when creating it?
Indeed, removed, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:337
+  HeaderVPBB->setName("vector.body");
+  HeaderVPBB->setParent(TheRegion);
+  TheRegion->setEntry(HeaderVPBB);
----------------
Ayal wrote:
> Is setting the parent of HeaderVPBB to TheRegion redundant - getOrCreateVPBB() should have done so already?
Indeed, removed, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:338
+  HeaderVPBB->setParent(TheRegion);
+  TheRegion->setEntry(HeaderVPBB);
 
----------------
Ayal wrote:
> Should this setEntry be done along with all others?
At the other point we would need extra checks for the block being a header.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:354
       assert(LI->getLoopFor(BB)->getHeader() == BB);
-      setRegionPredsFromBB(Region, BB);
+      if (TheRegion != Region)
+        setRegionPredsFromBB(Region, BB);
----------------
Ayal wrote:
> nit: may be simpler to handle the non-header case first, then potentially else-if the header-of-non-TheRegion case.
the non-header case should be handled first now


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:384
           Successors[0] == Region ? Successors[1] : Successors[0];
       Region->setExiting(VPBB);
       Region->setParent(ExitVPBB->getParent());
----------------
Ayal wrote:
> If Exits of regions are set here when identifying latches during successor setting, should Entries be set above when identifying headers during predecessor setting?
Entries are set at a different point now.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:87
+  for (BasicBlock *Pred : predecessors(BB)) {
+    // Ingore the pre-header, as it will be created and connected as part of the
+    // initial skeleton construction.
----------------
Ayal wrote:
> "it will be" >> "the vector-pre-header VPBB was"?
> 
> Avoid setting both predecessors of (any) header here, i.e., early-return if BB is a header block, to avoid setting edges to predecessor only to be discarded later?
> Likewise, avoid setting successors of latch blocks below, only to be discarded later.
> (Possibly as independent preparatory patch(es); make sure important orders are maintained.)
I had look at this and it turned out a bit more complicated, but overall simpler and easier to update/change in this patch: D159136


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:262
 void PlainCFGBuilder::buildPlainCFG() {
+  // Reuse the top-level region, preheader and exit VPBBs from the skeleton.
+  Loop2Region[TheLoop] = Plan.getVectorLoopRegion();
----------------
Ayal wrote:
> Ayal wrote:
> > nit: can assign `VPRegionBlock *TheRegion = Plan.getVectorLoopRegion();` and reuse it below to set
> > 
> > ```
> >   auto *VectorPreheaderVPBB = cast<VPBasicBlock>(TheRegion->getSinglePredecessor());
> >   BB2VPBB[ThePreheaderBB] = VectorPreheaderVPBB;
> >   BB2VPBB[LoopExitBB] = cast<VPBasicBlock>(TheRegion->getSingleSuccessor());
> > }
> > ```
> > 
> > Note that ThePreheaderBB is associated with both Plan.getPreheader() and Plan.getEntry(); here we're interested in the latter, which stands for Vector-Preheader.
> 
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:283
   // LoopBlocksDFS.
-  BasicBlock *ThePreheaderBB = TheLoop->getLoopPreheader();
-  assert((ThePreheaderBB->getTerminator()->getNumSuccessors() == 1) &&
-         "Unexpected loop preheader");
-  VPBasicBlock *ThePreheaderVPBB = Plan.getEntry();
-  BB2VPBB[ThePreheaderBB] = ThePreheaderVPBB;
   ThePreheaderVPBB->setName("vector.ph");
   for (auto &I : *ThePreheaderBB) {
----------------
Ayal wrote:
> 
Adjusted naming, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:289
   }
   // Create empty VPBB for Loop H so that we can link PH->H.
   VPBlockBase *HeaderVPBB = getOrCreateVPBB(TheLoop->getHeader());
----------------
Ayal wrote:
> drop suffix: PH->H is no longer linked, neither here nor elsewhere.
> 
> "Loop H" >> "the header block of the top region."
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:335
 
-  // 2. Process outermost loop exit. We created an empty VPBB for the loop
-  // single exit BB during the RPO traversal of the loop body but Instructions
-  // weren't visited because it's not part of the the loop.
-  BasicBlock *LoopExitBB = TheLoop->getUniqueExitBlock();
-  assert(LoopExitBB && "Loops with multiple exits are not supported.");
-  VPBasicBlock *LoopExitVPBB = BB2VPBB[LoopExitBB];
-  // Loop exit was already set as successor of the loop exiting BB.
-  // We only set its predecessor VPBB now.
-  setVPBBPredsFromBB(LoopExitVPBB, LoopExitBB);
-
   // 3. Fix up region blocks for loops. For each loop,
   //   * use the header block as entry to the corresponding region,
----------------
Ayal wrote:
> 
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:351
     VPBasicBlock *ExitingVPBB = getOrCreateVPBB(Exiting);
 
     // Disconnect backedge and pre-header from header.
----------------
Ayal wrote:
> Could the disconnects be avoided, and top region early continue?
This code is now gone, based on D159136


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:381
   }
   // 4. The whole CFG has been built at this point so all the input Values must
   // have a VPlan couterpart. Fix VPlan phi nodes by adding their corresponding
----------------
Ayal wrote:
> 
Adjusted, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158333