[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);
 }
----------------
setCondBit under 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()) {
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:733
+  case VPInstruction::ExitCheckAndBranch: {
+    if (Part == 0) {
+      Value *IV = State.get(getOperand(0), Part);
----------------
early-break if Part > 0?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:745
+          Cond,
+          cast<BranchInst>(State.CFG.LastBB->getTerminator())->getSuccessor(0),
+          State.CFG.VPBB2IRBB[Header]);
----------------
Once the exiting block is modelled in VPlan, it should help retrieve its IRBB directly (TODO).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:926
           isa<UnreachableInst>(LastBB->getTerminator())) &&
-         "Expected InnerLoop VPlan CFG to terminate with unreachable");
-  assert((!EnableVPlanNativePath || isa<BranchInst>(LastBB->getTerminator())) &&
-         "Expected VPlan CFG to terminate with branch in NativePath");
+         "Expected VPlan CFG to terminate with unreachable");
+
----------------
So if now LastBB is expected to terminate with unreachable rather than branch in native as well, remove the EnableVPlanNativePath from the assert?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:928
+
+  // Remove the Unreachable terminator from LastBB and then move both the branch
+  // and check to VectorLatchBB.
----------------
Should the unreachable terminator be removed from LastBB when the branch is introduced, rather than here? As in VPBranchOnMaskRecipe::execute()'s final ReplaceInstWithInst().

Could the compare stay in LastBB, soon to join its branch in VectorLatchBB when the two BB's merge?

It may be possible to associate LatchVPBB with VectorLatchBB upon construction and avoid createEmptyBasicBlock for it during VPBB::execute(), thereby having all latch recipes code-gen'd directly into VectorLatchBB, rather than having to merge separate LastBB and VectorLatchBB here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116479