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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4079
                         VPIteration(UF - 1, Lane));
     LCSSAPhi.addIncoming(lastIncomingValue, LoopMiddleBlock);
   }
----------------
Ayal wrote:
> Is this made redundant by ScalarExitValues?
It is in the latest version of the patch. One wrinkle is that we need to update the ScalarExitValue instructions in the the plan for the epilogue loop. At the moment this is done by removing the exit values referring the LCSSA phi's for the main loop's exits and adding exit values for the newly created exit block. 

That unfortunately requires exposing a bit more functionality to be able to do so.


================
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()) {
----------------
Ayal wrote:
> ScalarExitValue represents a (loop-closed) phi, and as such can be inserted at the top of MiddleVPBB?
Added a comment, thanks!


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8759
+          ExitPhi.getIncomingValueForBlock(OrigLoop->getLoopLatch());
+      VPValue *V = Plan.getOrAddVPValue(IncomingValue);
+      if (SkipExitValues.count(V))
----------------
Ayal wrote:
> >> 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?
Yes that would be an option. It is likely only relevant for handwritten tests.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10674
+                 BestEpiPlan.getVectorLoopRegion()->getSingleSuccessor()))) {
+          R.eraseFromParent();
+        }
----------------
Ayal wrote:
> Populate the exit block of the epilog vector loop with scalar exit value recipes, only to discard them all here?
The issue is that at this point, the exit values do not refer to the LCSSA phis created for the new exit block. I updated the code to re-add the exit values for the newly create exit block after executing the main plan. This allows removing fixLCSSAPHIs.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:333
     State->CFG.PrevBB = NewBB;
+    State->Builder.SetInsertPoint(NewBB, NewBB->begin());
   } else if (PrevVPBB && /* A */
----------------
Ayal wrote:
> 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?
We might need to insert extracts I think.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:821
+        State.get(getOperand(0), VPIteration(State.UF - 1, Lane)),
+        State.CFG.VPBB2IRBB[getParent()]);
+    break;
----------------
Ayal wrote:
> 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?
> 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(?)

The recipe doesn't create new LCSSA phis, but only updates existing ones. It may need to insert extracts, which should be placed in the middle block.

> 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?

I think the `exit` block can have multiple predecessors, in the test case it also has the latch of the scalar loop as predecessor. We could also generate an LCSSA phi in the middle block, but it doesn't seem needed currently.


================
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]]
----------------
Ayal wrote:
> This GEP_LCSSA phi was hooked-up to TMP2 by a ScalarExitValue, right?
Yep exactly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537