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

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8887
 
+  // Introduce recipes modeling the exit values.
+  Builder.setInsertPoint(MiddleBlock, MiddleBlock->getFirstNonPhi());
----------------
Ayal wrote:
> This stretches the already too long method to exceed 400 lines... please outline.
Moved to a separate function, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8894
+        continue;
+      VPValue *V = Plan->getOrAddVPValue(
+          ExitPhi.getIncomingValueForBlock(OrigLoop->getLoopLatch()));
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8901
+  // Remove exit value recipes for cases not handled yet. Those include exit
+  // values of induction and reduction chains and first-order recurrence phis.
+  for (VPRecipeBase &R : HeaderVPBB->phis()) {
----------------
Ayal wrote:
> Can alternatively first identify all VPValues in loop which are Not-To-Feed-Exit-VPInstruction, then wrap every middle-block loop-closed phi with an Exit VPInstruction provided it does not use an NTFEV VPValue? Instead of collecting NTFEV's and calling removeExitUsers() for each.
Updated to collect values to skip first, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:818
+    if (getParent()->getPlan()->isUniformAfterVectorization(getOperand(0)))
+      Lane = VPLane::getFirstLane();
+    ExitPhi->addIncoming(
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:822
+        State.CFG.VPBB2IRBB[getParent()]);
+
+    break;
----------------
Ayal wrote:
> nit: empty line?
Removed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537