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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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(
----------------
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.


================
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
----------------
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(?).


================
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);
 
----------------
nit: independent cleanup?


================
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());
----------------
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.


================
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.
----------------
"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.)


================
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();
----------------
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.


================
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:
> 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.



================
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) {
----------------



================
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());
----------------
drop suffix: PH->H is no longer linked, neither here nor elsewhere.

"Loop H" >> "the header block of the top region."


================
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,
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:351
     VPBasicBlock *ExitingVPBB = getOrCreateVPBB(Exiting);
 
     // Disconnect backedge and pre-header from header.
----------------
Could the disconnects be avoided, and top region early continue?


================
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
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158333