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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:510
   /// is provided, the integer induction variable will first be truncated to
   /// the corresponding type.
+  Instruction *widenIntOrFpInduction(PHINode *IV, const InductionDescriptor &ID,
----------------
Ayal wrote:
> Document return value?
Updated.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8684
         getRecipe(cast<Instruction>(PN->getIncomingValueForBlock(OrigLatch)));
-    R->addOperand(IncR->getVPSingleValue());
+    R->addOperand(IncR->getVPValue(0));
   }
----------------
Ayal wrote:
> Can `IncR->getVPSingleValue()` be retained?
yes!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1211
       // handled.
+      // There is also nothing to drop from VPWidenIntOrFpInductionRecipe.
       if (isa<VPWidenMemoryInstructionRecipe>(CurRec) ||
----------------
Ayal wrote:
> Ayal wrote:
> > fhahn wrote:
> > > 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.
> > Any other (header phi?) recipes worth excluding? (Independent of this patch)
> Is there still a problem (due to additional def/cast)?
> Prune/stop this backward slice at any header phi recipe?
> Is there still a problem (due to additional def/cast)?

No, with the recent improvements to cast handling there are no changes needed here!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:849
+        Cur = cast<Instruction>(Cur->getOperand(0));
+      PHINode *VecInd = cast<PHINode>(Cur);
+      // Move the last step to the end of the latch block. This ensures
----------------
Ayal wrote:
> Can VecInd be found instead by looking up State.get(IV..), as done for reductions and FOR rewired below?
Unfortunately that isn't possible, because `State.get(IV,...)` may not refer to the phi directly but may be reset to something other than the phi, e.g. in `CreateSplatIV `


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1015
+  /// modeling the increment  explicitly in VPlan.
+  Instruction *LastInduction = nullptr;
+
----------------
Ayal wrote:
> Better rename `LastInduction` to "LastIncrementOfVectorIV", throughout?
> 
> Scalar IV's are generated w/o a cross-iteration increment, based off of `Induction`, which is pre-generated as part of the skeleton, along with its last increment in the latch.
> 
> An alternative of forming a Region with an Exit to represent the latch, is mentioned in D114586.
> Better rename LastInduction to "LastIncrementOfVectorIV", throughout?

Done, thanks!

> An alternative of forming a Region with an Exit to represent the latch, is mentioned in D114586.

I'll follow up on the suggestion, but I don't see how this is related directly to this patch. Future patches will remove the need to keeping the last induction increment here by modeling the increment explicitly in VPlan. But to support the general induction case, we need to support expand the step for the induction from a SCEV expression in the pre-header. I'll put patches up to do this once we can model the pre-header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113183