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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9402
   if (CM.foldTailByMasking()) {
+    Builder.setInsertPoint(LatchVPBB, LatchVPBB->begin());
     for (VPRecipeBase &R : Plan->getEntry()->getEntryBasicBlock()->phis()) {
----------------
Ayal wrote:
> 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.
Updated the comment, thanks!


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


================
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 ";
----------------
Ayal wrote:
> Now we can assert number of users is positive?
There's still an edge case where this assert may not hold unfortunately: VPlan-stress testing, which exits before the recipes are introduced. Potentially buuildHCFG could already add the branch & mask recipe, but for now I left it as is.


================
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.
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > drop "Remove the Unreachable terminator"
> > Thanks, removed.
> typo: branchand 
fixed, thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116479