[PATCH] D116479: [VPlan] Introduce and use ExitCheckAndBranch VPInstruction.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9402
   if (CM.foldTailByMasking()) {
+    Builder.setInsertPoint(LatchVPBB, LatchVPBB->begin());
     for (VPRecipeBase &R : Plan->getEntry()->getEntryBasicBlock()->phis()) {
----------------
Ayal wrote:
> Ayal wrote:
> > Setting insert point once before the loop is fine; in addition, selects are now introduced at the beginning of the latch rather than at the end? Make sure live-out instruction Red appears before the latch?
> > Perhaps insert at/before Last(NonTerminal?) of latch , akin to FirstNonPhi?
> Make sure live-out instruction Red appears before the latch?
Added an assert. Was that what you had in mind?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:742
+    VPBasicBlock *Header = TopRegion->getEntry()->getEntryBasicBlock();
+    if (Header->empty())
+      Header = cast<VPBasicBlock>(Header->getSingleSuccessor());
----------------
Ayal wrote:
> IsVPlanNative? Perhaps it should use a subclass of VPRegionBlock which produces getHeader()/getExit() by bumping from Entry/Exit, in one place.
Added an assert. Maybe it would be better to get rid of the difference in the native path & model the pre-header block outside the vector loop region? (as follow-up down the line)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:930
+
+  // Remove the Unreachable terminator from LastBB and then move both the branch
+  // and check to VectorLatchBB.
----------------
Ayal wrote:
> drop "Remove the Unreachable terminator"
Thanks, removed.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:932
+  // and check to VectorLatchBB.
+  auto *LastBranch = cast<BranchInst>(LastBB->getTerminator());
+  LastBranch->moveBefore(VectorLatchBB->getTerminator());
----------------
Ayal wrote:
> fhahn wrote:
> > I looked into removing the separate latch block, but unfortunately `widenIntOrFpInduction` still relies on the latch block being created up-front:
> > 
> > ```
> >   // Move the last step to the end of the latch block. This ensures consistent
> >   // placement of all induction updates.
> >   auto *LoopVectorLatch = LI->getLoopFor(LoopVectorBody)->getLoopLatch();
> >   auto *Br = cast<BranchInst>(LoopVectorLatch->getTerminator());
> >   LastInduction->moveBefore(Br);
> >   LastInduction->setName("vec.ind.next");
> > 
> > ```
> > 
> > Reusing the created vector latch for the exit block of the region is also not straight-forward, because we may need to look through multiple single successors until we reach a the exit VPBB. This is complicated by blocks inside replicate regions, where we would need to check the successor of the region, but *only* for the last iteration in the loop for the region.
> > 
> > So I think it is simpler to keep things as is, until the VPWidenIntOrFpInduction increments are modeled separately.
> Fine. If this too-early sinking of LastInduction is the only reason for creating the latch block up-front, then let's remember (TODO?) to remove the latter once the former is modelled with a recipe properly placed in the Exit VPBB, next to ExitCheckAndBranch.
Added a todo.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:938
+  cast<Instruction>(LastBranch->getCondition())
+      ->moveBefore(VectorLatchBB->getTerminator());
   // Connect LastBB to VectorLatchBB to facilitate their merge.
----------------
Ayal wrote:
> moveBefore(LastBranch) ?
That's better, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:180
+  auto *LastInst = dyn_cast<VPInstruction>(std::prev(Exit->end()));
+  if (Exit->begin() == Exit->end() || !LastInst ||
+      LastInst->getOpcode() != VPInstruction::ExitCheckAndBranch) {
----------------
Ayal wrote:
> Exit->empty()? Check above along with !Exit, to avoid taking prev of Exit->end()?
I added a lambda for the VPInstruction check and updated to use `empty()`. With the check above the error message may be a bit misleading or would need duplicating.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116479