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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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,
----------------
Document return value?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2517
+                                           State);
   }
 
----------------
{} can be dropped


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1211
       // handled.
+      // There is also nothing to drop from VPWidenIntOrFpInductionRecipe.
       if (isa<VPWidenMemoryInstructionRecipe>(CurRec) ||
----------------
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?


================
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
----------------
Can VecInd be found instead by looking up State.get(IV..), as done for reductions and FOR rewired below?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1015
+  /// modeling the increment  explicitly in VPlan.
+  Instruction *LastInduction = nullptr;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113183