[PATCH] D126679: [VPlan] Update vector latch terminator edge to exit block after execution.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 2 04:51:25 PDT 2022
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
More information about the llvm-commits
mailing list