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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I just realized I forgot to submit my responses to the latest comments. Sorry about that and here they are :)



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3099
+  // Update dominator for loop exit. During skeleton creation, only the vector
+  // pre-heaader and the middle block are created. The vector loop is entirely
+  // created during VPlan exection.
----------------
Ayal wrote:
> typo: heaader 
Fixed, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8749
+
   VPBasicBlock *HeaderVPBB = new VPBasicBlock();
   VPBasicBlock *LatchVPBB = new VPBasicBlock("vector.latch");
----------------
Ayal wrote:
> (nit: could we initialize HeaderVPBB's name to "vector.body" here, next to vector.latch and vector.ph, instead of overwriting it below, provided we guard setting VPBB's name to BB->getName() if VPBB != HeaderVPBB?)
updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3103
   ReplaceInstWithInst(LoopMiddleBlock->getTerminator(), BrInst);
 
   // Update dominator for loop exit.
----------------
Ayal wrote:
> 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? 
Update, thanks!


================
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};
----------------
Ayal wrote:
> Comment deserves updating?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8709
 
   // Create initial VPlan skeleton, with separate header and latch blocks.
   VPBasicBlock *HeaderVPBB = new VPBasicBlock();
----------------
Ayal wrote:
> 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);
> 
> ```
Done, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:343
         PrevVPBB->getSingleHierarchicalSuccessor() &&
-        (SingleHPred->getParent() == GetEnclosingNonReplicateRegion(this) &&
+        (SingleHPred->getParent() == getEnclosingLoopRegion() &&
          !IsNonReplicateR(SingleHPred))) &&      /* B */
----------------
Ayal wrote:
> getEnclosingLoopRegion() assumes/asserts we're in a loop; perhaps have it return null if we're not?
I think it should return null if the block doesn't have parent region. The assertion at the moment ensures we do not have nested replicate region.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:431
+    P = P->getParent();
+    assert(!cast<VPRegionBlock>(P)->isReplicator());
+  }
----------------
Ayal wrote:
> Add error message to the assert.
Added!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:938
 /// Generate the code inside the body of the vectorized loop. Assumes a single
 /// LoopVectorBody basic-block was created for this. Introduce additional
 /// basic-blocks as needed, and fill them all.
----------------
Ayal wrote:
> Update comment.
> "inside the body" >> "inside the preheader and body"
> "Assumes a single LoopVectorBody basic-block was created for this" - a single preheader basic-block?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1032
 
+  BasicBlock *VectorHeaderBB = State->CFG.VPBB2IRBB[Header];
   // We do not attempt to preserve DT for outer loop vectorization currently.
----------------
Ayal wrote:
> place below inside the 'if'?
Moved thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1481
+
+  BasicBlock *VectorPH = State.CFG.getPreheaderBBFor(this);
+  Value *Res =
----------------
Ayal wrote:
> Do we need to getPreheaderBBFor(this), given that the recipe should reside in the preheader VPBB?
> 
> 
This patch doesn't yet move any recipes. The linked follow-up patches will perform the moves, including D122095 and children.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:249
+BasicBlock *VPTransformState::CFGState::getPreheaderBBFor(VPBasicBlock *VPBB) {
+  return VPBB2IRBB[VPBB->getSingleHierarchicalPredecessor()
+                       ->getExitBasicBlock()];
----------------
Ayal wrote:
> 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?
Good points, updated to take a recipe, and use `getEnclosingLoopRegion` helper.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:921
 
   // Remove the edge between Header and Latch to allow other connections.
   // Temporarily terminate with unreachable until CFG is rewired.
----------------
Ayal wrote:
> "between Header and Latch" >> between PreHeader and its successor/Header ?
I think this stale comment is gone now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121624