[PATCH] D116479: [VPlan] Introduce and use BranchOnCount VPInstruction.

[Ayal Zaks via Phabricator via llvm-commits]

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

This looks good to me, adding minor optional suggestions.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9402
   if (CM.foldTailByMasking()) {
+    Builder.setInsertPoint(LatchVPBB, LatchVPBB->begin());
     for (VPRecipeBase &R : Plan->getEntry()->getEntryBasicBlock()->phis()) {
----------------
fhahn wrote:
> Ayal wrote:
> > Ayal wrote:
> > > Setting insert point once before the loop is fine; in addition, selects are now introduced at the beginning of the latch rather than at the end? Make sure live-out instruction Red appears before the latch?
> > > Perhaps insert at/before Last(NonTerminal?) of latch , akin to FirstNonPhi?
> > Make sure live-out instruction Red appears before the latch?
> Added an assert. Was that what you had in mind?
Yeah, or set the insert point to the end of LatchVPBB (prior to its BranchOnCount terminator) rather than the beginning of LatchVPBB, retaining original behavior; conceptually, the reduction recipe should be allowed to reside (earlier) in the latch.
Above comment "at the end of the latch" deserves updating.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:739
+    Value *Cond = Builder.CreateICmpEQ(IV, TC);
+
+    auto *Plan = getParent()->getPlan();
----------------
Add a comment that after creating the compare we now create the branch?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:754
+        State.CFG.VPBB2IRBB[Header]);
+    Builder.GetInsertBlock()->getTerminator()->eraseFromParent();
+    break;
----------------
This is fine, can alternatively use ReplaceInstWithInst() as does VPBranchOnMask::execute.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:996
 
-  assert(VectorTripCount.getNumUsers() == 0 &&
-         "should not be used yet in VPlan");
+  if (VectorTripCount.getNumUsers() > 0) {
+    O << "\nLive-in ";
----------------
Now we can assert number of users is positive?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:742
+    VPBasicBlock *Header = TopRegion->getEntry()->getEntryBasicBlock();
+    if (Header->empty())
+      Header = cast<VPBasicBlock>(Header->getSingleSuccessor());
----------------
fhahn wrote:
> Ayal wrote:
> > IsVPlanNative? Perhaps it should use a subclass of VPRegionBlock which produces getHeader()/getExit() by bumping from Entry/Exit, in one place.
> Added an assert. Maybe it would be better to get rid of the difference in the native path & model the pre-header block outside the vector loop region? (as follow-up down the line)
> Added an assert. Maybe it would be better to get rid of the difference in the native path & model the pre-header block outside the vector loop region? (as follow-up down the line)
+1


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:930
+
+  // Remove the Unreachable terminator from LastBB and then move both the branch
+  // and check to VectorLatchBB.
----------------
fhahn wrote:
> Ayal wrote:
> > drop "Remove the Unreachable terminator"
> Thanks, removed.
typo: branchand 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:745
+          Cond,
+          cast<BranchInst>(State.CFG.LastBB->getTerminator())->getSuccessor(0),
+          State.CFG.VPBB2IRBB[Header]);
----------------
fhahn wrote:
> Ayal wrote:
> > Once the exiting block is modelled in VPlan, it should help retrieve its IRBB directly (TODO).
> I added the todo, thanks!
Worth setting `BasicBlock *ExitBlock = `... next to setting `Header` above, and place comment there?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:180
+  auto *LastInst = dyn_cast<VPInstruction>(std::prev(Exit->end()));
+  if (Exit->begin() == Exit->end() || !LastInst ||
+      LastInst->getOpcode() != VPInstruction::ExitCheckAndBranch) {
----------------
fhahn wrote:
> Ayal wrote:
> > Exit->empty()? Check above along with !Exit, to avoid taking prev of Exit->end()?
> I added a lambda for the VPInstruction check and updated to use `empty()`. With the check above the error message may be a bit misleading or would need duplicating.
Spit out "exit block must not be empty" if Exit.empty() and check the inlined lambda afterwards?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116479