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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:260
       TermBr->setSuccessor(0, NewBB);
-    } else if (PredVPSuccessors.size() == 2) {
+    } else {
       unsigned idx = PredVPSuccessors.front() == this ? 0 : 1;
----------------
Ayal wrote:
> nit: assert and use TermBr here?
> 
> Can comment that this scan sets each forward successor here when it is created, and excludes backedges. A backward successor is set when the branch is created (as successor 1).
Done & comment added, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:293
+    BasicBlock *ExitingBB = State->CFG.VPBB2IRBB[ExitingVPBB];
+    cast<BranchInst>(ExitingBB->getTerminator())->setSuccessor(0, NewBB);
   } else if (PrevVPBB && /* A */
----------------
Ayal wrote:
> Can comment that Exit block of a loop is always set to be successor 0 of the Exiting block.
added the comment, thanks!


================
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(),
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > 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?
> > > Here there's only a single destination/successor 0: "destination(s) later when they are" >> "destination later when it is".
> > 
> > Updated! I also updated the comment to mention backward dest = header, forward dest = exit/middle block.
> > 
> > > Setting IfTrue first to Builder.GetInsertBlock() and then replacing it with nullptr to circumvent an assert of CreateCondBr?
> > 
> > Yes, unfortunately the first destination is required to be  valid block.
> >> Setting IfTrue first to Builder.GetInsertBlock() and then replacing it with nullptr to circumvent an assert of CreateCondBr?
> 
> > Yes, unfortunately the first destination is required to be valid block.
> 
> Understood. Might be worth a comment.
Extended the comment, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126679