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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8940
+  EB->appendRecipe(ExitCheckAndBranch);
+  EB->setCondBit(nullptr);
 }
----------------
fhahn wrote:
> Ayal wrote:
> > setCondBit under IsVPlanNative?
> Updated, thanks!
Fold it under the above "if (IsVPlanNative)"?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:742
+    VPBasicBlock *Header = TopRegion->getEntry()->getEntryBasicBlock();
+    if (Header->empty())
+      Header = cast<VPBasicBlock>(Header->getSingleSuccessor());
----------------
IsVPlanNative? Perhaps it should use a subclass of VPRegionBlock which produces getHeader()/getExit() by bumping from Entry/Exit, in one place.


================
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.
----------------
drop "Remove the Unreachable terminator"


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:932
+  // and check to VectorLatchBB.
+  auto *LastBranch = cast<BranchInst>(LastBB->getTerminator());
+  LastBranch->moveBefore(VectorLatchBB->getTerminator());
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:938
+  cast<Instruction>(LastBranch->getCondition())
+      ->moveBefore(VectorLatchBB->getTerminator());
   // Connect LastBB to VectorLatchBB to facilitate their merge.
----------------
moveBefore(LastBranch) ?


================
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) {
----------------
Exit->empty()? Check above along with !Exit, to avoid taking prev of Exit->end()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116479