[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:276
              "the current block must be the single successor of the region");
       PredBBTerminator->setSuccessor(0, NewBB);
     }
----------------
Ayal wrote:
> 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.
> 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?

Exactly! I had another look and the extra code here to handle the exiting block separately is not needed any longer I think with this patch.

It can be handled in the else case above, if we use `getHierarchicalSuccessors` instead of `getSuccessors`. This seems simpler and I updated the patch in that respect.


================
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();
----------------
Ayal wrote:
> PredVPBB >> PredVPBlock or ExitingVPBlock? (Expecting BB to mean BasicBlock rather than BlockBase)
Changed to `PredVPB`, which I think is used elsewhere.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:302
+    VPBasicBlock *ExitingVPBB = PredVPBB->getExitingBasicBlock();
+    assert(PredVPBB->getSuccessors()[0] == this);
+    BasicBlock *ExitingBB = State->CFG.VPBB2IRBB[ExitingVPBB];
----------------
Ayal wrote:
> Add error message?
Done, also switched to using `getSingleSuccessor`


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:786
     VPBasicBlock *Header = TopRegion->getEntry()->getEntryBasicBlock();
     if (Header->empty()) {
       assert(EnableVPlanNativePath &&
----------------
Ayal wrote:
> (nit: is this still needed, or subject to future cleanup?)
It's not needed any longer, removed in 4f1c86e3d5ef, 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126679