[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:8940
+  EB->appendRecipe(ExitCheckAndBranch);
+  EB->setCondBit(nullptr);
 }
----------------
Ayal wrote:
> setCondBit under IsVPlanNative?
Updated, thanks!


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


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


================
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");
+
----------------
Ayal wrote:
> So if now LastBB is expected to terminate with unreachable rather than branch in native as well, remove the EnableVPlanNativePath from the assert?
Removed VPlanNativePath from assert, thanks!


================
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.
----------------
Ayal wrote:
> 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.
> Should the unreachable terminator be removed from LastBB when the branch is introduced, rather than here? As in VPBranchOnMaskRecipe::execute()'s final ReplaceInstWithInst().

Updated, thanks!

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

It could, but it would require updating ~50 tests. The reason is that widenIntOrFpInduction will still generate the vector induction increments in VectorLatchBB, which is outside VPlan.

> 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.

It seems like this would require threading through VectorLatchBB to VPBasicBlock::execute. Not sure if that extra work is justified for the temporary workaround that won't be needed once the latch block is modeled in VPlan directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116479