[PATCH] D121624: [VPlan] Model pre-header explicitly.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3103
   ReplaceInstWithInst(LoopMiddleBlock->getTerminator(), BrInst);
 
   // Update dominator for loop exit.
----------------
Worth commenting, say in the summary, that the skeleton is left creating only the preheader and the middle-block (and the scalar loop preheader), with the "vector.body" header-block now being created by VPlan::execute? 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7575
 
   // 1. Create a new empty loop. Unlink the old loop and connect the new one.
   VPTransformState State{BestVF, BestUF, LI, DT, ILV.Builder, &ILV, &BestVPlan};
----------------
Comment deserves updating?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8709
 
   // Create initial VPlan skeleton, with separate header and latch blocks.
   VPBasicBlock *HeaderVPBB = new VPBasicBlock();
----------------
fhahn wrote:
> Ayal wrote:
> > Comment deserves updating.
> > 
> > Start by creating Preheader, then Plan, and later insert TopRegion after Preheader?
> Added comment with up-to-date info about the initial skeleton.
> Start by creating Preheader, then Plan, and later insert TopRegion after Preheader?
suggestion was, i.e., to have

```
  VPBasicBlock *Preheader = new VPBasicBlock("vector.ph");
  auto Plan = std::make_unique<VPlan>(Preheader);

  VPBasicBlock *HeaderVPBB = new VPBasicBlock();
  VPBasicBlock *LatchVPBB = new VPBasicBlock("vector.latch");
  VPBlockUtils::insertBlockAfter(LatchVPBB, HeaderVPBB);
  auto *TopRegion = new VPRegionBlock(HeaderVPBB, LatchVPBB, "vector loop");
  VPBlockUtils::insertBlockAfter(TopRegion, Preheader);

```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:491
+    BasicBlock *VectorPH =
+        State->CFG.VPBB2IRBB[getSinglePredecessor()->getExitBasicBlock()];
+    Loop *ParentLoop = State->LI->getLoopFor(VectorPH);
----------------
VPBB2IRBB[getPreheaderVPBB()]? (suggested above)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1460
+  VPBasicBlock *PreheaderVPBB =
+      getParent()->getSingleHierarchicalPredecessor()->getExitBasicBlock();
+  EntryPart->addIncoming(Start, State.CFG.VPBB2IRBB[PreheaderVPBB]);
----------------
getPreheaderBBFor()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:249
+BasicBlock *VPTransformState::CFGState::getPreheaderBBFor(VPBasicBlock *VPBB) {
+  return VPBB2IRBB[VPBB->getSingleHierarchicalPredecessor()
+                       ->getExitBasicBlock()];
----------------
This assumes VPBB is the header.
Can alternatively climb from VPBB to its enclosing loop region (ensure there is one), and take its predecessor; that will only assume VPBB belongs to a loop, which is implied by asking for its preheader.
All uses actually have a recipe to provide, can take it's parent VPBB here.
Perhaps a loop region should provide its preheader VPBB?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121624