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

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1211
       // handled.
+      // There is also nothing to drop from VPWidenIntOrFpInductionRecipe.
       if (isa<VPWidenMemoryInstructionRecipe>(CurRec) ||
----------------
Ayal wrote:
> Is this addition of excluding VPWidenIntOrFpInductionRecipe independent of this patch?
I think it makes sense to exclude it in any case, but it is becoming problematic with the patch (due to adding additional defs, causing a crash in `getUndderlyingValue). I *think* it could also cause issues if the induction recipe has a CastDef.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2447
+  State.set(StepDef, LastInduction, 0);
+  State.set(PhiDef, VecInd, 0);
   VecInd->addIncoming(SteppedStart, LoopVectorPreHeader);
----------------
Ayal wrote:
> `PhiDef` is redundant? `VecInd` is already being State.set above as part 0 of `Def`.
Unfortunately `Def` may be reset to something other than the phi, e.g. in `CreateSplatIV `.


================
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));
----------------
Ayal wrote:
> 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<>.
I changed it to `isa<>`.

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

Agreed, but could/should be a separate change?



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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1070
+
+  InductionDescriptor &getInductionDescriptor() { return II; }
 };
----------------
Ayal wrote:
> who needs the II?
No, it is a leftover from an earlier version. Removed all II-related changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113183