[PATCH] D126679: [VPlan] Update vector latch terminator edge to exit block after execution.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:276
              "the current block must be the single successor of the region");
       PredBBTerminator->setSuccessor(0, NewBB);
     }
----------------
Set here only fallthru successor 0 to NewBB, or only backedge successor 1 to Header?

Intent is to set the backward successor of a conditional (loop) branch when the branch is created, but to set forward successor(s) later when they are created/visited, for all conditional branches? All such successors will not be able to reuse previous BB so must call createEmptyBasicBlock() except for reusing ExitBB.

Note that LoopRegion in this case refers to a nested loop rather than the outer/top/vector loop - createEmptyBasicBlock() call below excludes final Exit VPBB.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:300
+    // Update the branch instruction in the predecessor to branch to ExitBB.
+    VPBlockBase *PredVPBB = getSingleHierarchicalPredecessor();
+    VPBasicBlock *ExitingVPBB = PredVPBB->getExitingBasicBlock();
----------------
PredVPBB >> PredVPBlock or ExitingVPBlock? (Expecting BB to mean BasicBlock rather than BlockBase)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:302
+    VPBasicBlock *ExitingVPBB = PredVPBB->getExitingBasicBlock();
+    assert(PredVPBB->getSuccessors()[0] == this);
+    BasicBlock *ExitingBB = State->CFG.VPBB2IRBB[ExitingVPBB];
----------------
Add error message?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:769
       CondBr = Builder.CreateCondBr(Cond, Builder.GetInsertBlock(), nullptr);
     }
     CondBr->setSuccessor(0, nullptr);
----------------
(nit: can set IfFalse to null for both above, then setSuccessor(1) to Header if parent isLatch()), as we're setting Successor(0)...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:786
     VPBasicBlock *Header = TopRegion->getEntry()->getEntryBasicBlock();
     if (Header->empty()) {
       assert(EnableVPlanNativePath &&
----------------
(nit: is this still needed, or subject to future cleanup?)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:794
+    // branch, hooking it up to backward destination now and to forward
+    // destination(s) later when they are created.
+    BranchInst *CondBr = Builder.CreateCondBr(Cond, Builder.GetInsertBlock(),
----------------
Here there's only a single destination/successor 0: "destination(s) later when they are" >> "destination later when it is".

Setting IfTrue first to Builder.GetInsertBlock() and then replacing it with nullptr to circumvent an assert of CreateCondBr?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126679