[PATCH] D113223: [VPlan] Add VPCanonicalIVRecipe, partly retire createInductionVariable.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8919
+// latch.
+static void addCanonicalIVPHIRecipes(VPlan &Plan, Type *IdxTy, DebugLoc DL,
+                                     bool HasNUW, bool IsVPlanNative) {
----------------
addCanonicalIVPHIRecipes >> addCanonicalIVRecipes (one is a PHI, the other is not)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:927
+    if (isa<VPWidenIntOrFpInductionRecipe>(&R) || isa<VPWidenPHIRecipe>(&R) ||
+        isa<VPWidenCanonicalIVRecipe>(&R))
+      continue;
----------------
>     if (isa<VPWidenIntOrFpInductionRecipe>(&R) || isa<VPWidenPHIRecipe>(&R) ||
>        isa<VPWidenCanonicalIVRecipe>(&R))
>       continue;

Reasoning behind this early-exit:
createVectorIntOrFpInductionPHI()/widenPHIInstruction() take care of generating LastInduction/InductionGEP and rewiring them back to the phi during VPWidenIntOrFpInductionRecipe/VPWidenPHIRecipe::execute, respectively. Once these values are recipe'd, they too c/should be rewired here instead (TODO).
VPWidenCanonicalIVRecipe depends on the phi of VPCanonicalIVRecipe (which is rewired below) rather than having its own phi. It represents an original IR phi that turns into a non-IR instruction when vectorized, similar to VPBlendRecipe, which should appear right after "real" phi recipes. Should they be placed outside VPBB->phis()? Should both be supported, e.g., VPBB->real-phis() and VPBB->aliased-phis()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:931
+    auto *PhiR = cast<VPHeaderPHIRecipe>(&R);
+    // For first-order recurrences and in-order reduction phis, only a single
+    // part is generated, which provides the last part from the previous
----------------
"// For first-order recurrences [, canonical IV] and in-order reduction phis ..."

"Otherwise" - for non ordered reductions,


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:933
+                       ? State->get(R.getVPSingleValue(), VPIteration(Part, 0))
+                       : State->get(PhiR, Part);
+      Value *Val =
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > Storing per-part 0 rather than per-lane 0 and common base class should lead to a common State->get(PhiR, Part) for all, so the loop becomes
> > > > 
> > > > ```
> > > >     for (unsigned Part = 0; Part < NumPartsForNewPhi; ++Part) {
> > > >       Value *Phi = State->get(PhiR, Part);
> > > >       Value *Val = State->get(BackedgeValue, IsSinglePart ? SinglePart : Part);
> > > >       cast<PHINode>(Phi)->addIncoming(Val, VectorLatchBB);
> > > >     }
> > > > ```
> > > Storing a scalar value in the 'vector' part of State seems not ideal, but it does indeed simplify things a lot here. Updated
> > That is how uniform (/UAV) scalar values are stored in general, namely, in the 'vector' part of State.
> > The scalar canonical IV is conceptually UAV, only its lane 0 is used.
> One issue with that representation is that it is not possible to get a vector with the scalar splatted I think. Not an issue at the moment here though.
It should be possible to obtain any vector, stored per-part; any uniform scalar, stored per-part; and any non-uniform scalar, stored per-lane.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:728
+  case VPInstruction::InductionIncrementNUW: {
+    if (Part == 0) {
+      bool IsNUW = getOpcode() == VPInstruction::InductionIncrementNUW;
----------------
Ayal wrote:
> early-break if Part != 0
Ah, now it looks more logical to first handle Part 0; and possibly fuse the last State.set & break.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:178
   }
 
   for (const VPRegionBlock *Region :
----------------
fhahn wrote:
> Ayal wrote:
> > Verify that the last recipe in Exit is a CanonicalIVIncrement VPInstruction?
> I am not sure this is something we should enforce in the verifier, as it is easy to retrieve from the canonical IV.
That's fine. Conceptually, the phi of the canonical IV can be placed anywhere among the header phis, although it's good to canonicalize its position to be first. OTOH, the inc-cmp-br of the canonical IV must be placed at the end of the latch, so it would be more robust to rely on it and its user(s) instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113223