[PATCH] D123537: [VPlan] Model first exit values in VPlan.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4079
                         VPIteration(UF - 1, Lane));
     LCSSAPhi.addIncoming(lastIncomingValue, LoopMiddleBlock);
   }
----------------
Is this made redundant by ScalarExitValues?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8750
+  // Introduce recipes modeling the exit values.
+  Builder.setInsertPoint(MiddleVPBB, MiddleVPBB->getFirstNonPhi());
+  if (BasicBlock *ExitBB = OrigLoop->getUniqueExitBlock()) {
----------------
ScalarExitValue represents a (loop-closed) phi, and as such can be inserted at the top of MiddleVPBB?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8751
+  Builder.setInsertPoint(MiddleVPBB, MiddleVPBB->getFirstNonPhi());
+  if (BasicBlock *ExitBB = OrigLoop->getUniqueExitBlock()) {
+    for (PHINode &ExitPhi : ExitBB->phis()) {
----------------
Early-exit if loop has multiple exit blocks, at the outset?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8754
+      // Skip multi-exit loops for now.
+      if (ExitPhi.getNumIncomingValues() != 1)
+        continue;
----------------
Early-exit instead, if ExitBB has more that one predecessor, at the outset?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8759
+          ExitPhi.getIncomingValueForBlock(OrigLoop->getLoopLatch());
+      VPValue *V = Plan.getOrAddVPValue(IncomingValue);
+      if (SkipExitValues.count(V))
----------------
>> This should get rather than add, right?

> We could have loop invariant incoming values which won't be modeled in VPlan, hence getOrAddVPValue.

Hmm, IncomingValue is initially an IR Instruction inside the loop feeding a loop-closed phi; if it is loop invariant (missed pre-LV LICM) then LV could LICM and move it to the pre-header (analogous to hoisting up a live-out IV) but instead VPlan keeps it in place wrapping it up with a uniform ReplicateRecipe?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10674
+                 BestEpiPlan.getVectorLoopRegion()->getSingleSuccessor()))) {
+          R.eraseFromParent();
+        }
----------------
Populate the exit block of the epilog vector loop with scalar exit value recipes, only to discard them all here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8894
+        continue;
+      VPValue *V = Plan->getOrAddVPValue(
+          ExitPhi.getIncomingValueForBlock(OrigLoop->getLoopLatch()));
----------------
fhahn wrote:
> Ayal wrote:
> > This should get rather than add, right?
> > 
> > Check if V is a header phi recipe, whose exit values are not represented yet?
> > 
> > Exit values of inductions are disconnected from the loop body by precomputing them in preheader, so modeling them as VPInstructions in the middle block would require also modeling their precomputation in the preheader?
> > This should get rather than add, right?
> 
> We could have loop invariant incoming values which won't be modeled in VPlan, hence getOrAddVPValue.
> 
> > Exit values of inductions are disconnected from the loop body by precomputing them in preheader, so modeling them as VPInstructions in the middle block would require also modeling their precomputation in the preheader?
> 
> We should be able to add those to the pre-header, but I think it's best to do it one step at a time?
>> Exit values of inductions are disconnected from the loop body by precomputing them in preheader, so modeling them as VPInstructions in the middle block would require also modeling their precomputation in the preheader?

> We should be able to add those to the pre-header, but I think it's best to do it one step at a time?

Sure, by all means.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:333
     State->CFG.PrevBB = NewBB;
+    State->Builder.SetInsertPoint(NewBB, NewBB->begin());
   } else if (PrevVPBB && /* A */
----------------
Sounds right, but is this really needed - ScalarExitValue recipes seem to only hook-up loop-closed phis (at this time), w/o generating any instructions using Builder?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:813
+  case VPInstruction::ScalarExitValue: {
+    if (Part != 0)
+      break;
----------------
nit: seems more logical to generate code when called for the last part rather than the first, bailing out here `if (Part != State.UF - 1)`, and using Part later rather than State.UF - 1?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:821
+        State.get(getOperand(0), VPIteration(State.UF - 1, Lane)),
+        State.CFG.VPBB2IRBB[getParent()]);
+    break;
----------------
Placing ScalarExitValues inside middleBlock suggests they represent loop-closed phi's that will appear there, but actually the loop-closed phi's reside all along in Exit block, and use ScalarExitValue to retrieve its parent - middleBlock when rewiring its operand(?)

Test pr51366 below shows a ScalarExitValue inside 'exit' block which follows 'middleBlock'. Wonder if they are meant to be fused, provided 'exit' block has no other predecessor, or should ScalarExitValue introduce a loop-closed phi in middleBlock instead?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:818
+    if (getParent()->getPlan()->isUniformAfterVectorization(getOperand(0)))
+      Lane = VPLane::getFirstLane();
+    ExitPhi->addIncoming(
----------------
fhahn wrote:
> Ayal wrote:
> > Indicate that ExitValue usesScalars()?
> > Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.
> > Indicate that ExitValue usesScalars()?
> 
> Added `usesScalars` implementation.
> 
> > Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.
> 
> Good point, not sure what the ideal name would be. Changed `ScalarExitValue` but I am not sure if that is much better. They model all exit values that do not need special handling.
>>Indicate that ExitValue usesScalars()?
> Added usesScalars implementation.
It actually usesFirstOrLastLane only... perhaps something to think of in the future.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:818
+    if (getParent()->getPlan()->isUniformAfterVectorization(getOperand(0)))
+      Lane = VPLane::getFirstLane();
+    ExitPhi->addIncoming(
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Indicate that ExitValue usesScalars()?
> > > Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.
> > > Indicate that ExitValue usesScalars()?
> > 
> > Added `usesScalars` implementation.
> > 
> > > Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.
> > 
> > Good point, not sure what the ideal name would be. Changed `ScalarExitValue` but I am not sure if that is much better. They model all exit values that do not need special handling.
> >>Indicate that ExitValue usesScalars()?
> > Added usesScalars implementation.
> It actually usesFirstOrLastLane only... perhaps something to think of in the future.
>> Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.

> Good point, not sure what the ideal name would be. Changed ScalarExitValue but I am not sure if that is much better. They model all exit values that do not need special handling.

Hmm, it seems ScalarExitValue merely models/wraps an IR loop-closed-ssa-phi taking care only of hooking it's operand. Perhaps deserves a "VPLiveOut" or "VPLoopClosedPhi" subclass of VPUser?


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/pr51366-sunk-instruction-used-outside-of-loop.ll:44
+; CHECK:       exit:
+; CHECK-NEXT:    [[GEP_LCSSA:%.*]] = phi i32* [ %gep.src, %loop.latch ], [ [[TMP2]], %middle.block ]
+; CHECK-NEXT:    ret i32* [[GEP_LCSSA]]
----------------
This GEP_LCSSA phi was hooked-up to TMP2 by a ScalarExitValue, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537