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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Ship it!



================
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;
----------------
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).


================
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 */
----------------
Can comment that Exit block of a loop is always set to be successor 0 of the Exiting block.


================
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(),
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126679