[PATCH] D113183: [LV] Patch up induction phis after VPlan execution.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In D113183#3151729 <https://reviews.llvm.org/D113183#3151729>, @fhahn wrote:

> Rebased, all tests should be passing now!
>
> In D113183#3146716 <https://reviews.llvm.org/D113183#3146716>, @Ayal wrote:
>
>> Would it be easier to first keep LastInduction in its original place in the header and then sink it to the latch as a VPlan-to-VPlan transformation after all blocks have been formed? Such a patch should refer to the original D22416 <https://reviews.llvm.org/D22416>.
>
> In this patch, there is still no recipe to represent LastInduction, so unfortuantely I don't think it could be sunk just yet. But D113223 <https://reviews.llvm.org/D113223> (next patch in stack after the current one) introduces new VPInstruction opcodes `InductionIncrementNUW`/`InductionIncrement` and uses them to explicitly model LastInduction.
>
> he patch then can simplify add it to the last block of the plan. The exit condition & branch get a similar treatment in D113224 <https://reviews.llvm.org/D113224>.

Yes, in order to sink LastInduction it should be wrapped in a recipe by itself, separating it from VPWidenIntOrFpInductionRecipe. Would introducing such a LastInduction recipe here, potentially created in FixHeaderPhis out of its feeding/consuming VPWidenIntOrFpInductionRecipe, be simpler for this patch? It needs only to take of

  LastInduction = cast<Instruction>(
      Builder.CreateBinOp(AddOp, LastInduction, SplatVF, "step.add"));
  LastInduction->setDebugLoc(EntryVal->getDebugLoc());

keeping VPWidenIntOrFpInductionRecipe with a single value Def.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1211
       // handled.
+      // There is also nothing to drop from VPWidenIntOrFpInductionRecipe.
       if (isa<VPWidenMemoryInstructionRecipe>(CurRec) ||
----------------
Is this addition of excluding VPWidenIntOrFpInductionRecipe independent of this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2447
+  State.set(StepDef, LastInduction, 0);
+  State.set(PhiDef, VecInd, 0);
   VecInd->addIncoming(SteppedStart, LoopVectorPreHeader);
----------------
`PhiDef` is redundant? `VecInd` is already being State.set above as part 0 of `Def`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9166
         getRecipe(cast<Instruction>(PN->getIncomingValueForBlock(OrigLatch)));
-    R->addOperand(IncR->getVPSingleValue());
+    if (auto *IV = dyn_cast<VPWidenIntOrFpInductionRecipe>(IncR))
+      R->addOperand(IncR->getVPValue(0));
----------------
This is when an induction feeds a reduction/FOR; the induction became a multi-valued def so we want to select its first, original value?

Perhaps Inductions should be treated as Reductions, included in PhisToFix, and their increment recipe introduced and rewired here?

IV isn't used, suffice to ask isa<>.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9526
           auto *UV = Def->getUnderlyingValue();
-          Plan->addVPValue(UV, Def);
+          if (UV)
+            Plan->addVPValue(UV, Def);
----------------
(Related to introducing additional values to a Def; having no underlying values?)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1070
+
+  InductionDescriptor &getInductionDescriptor() { return II; }
 };
----------------
who needs the II?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113183