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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn 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) {
----------------
Ayal wrote:
> addCanonicalIVPHIRecipes >> addCanonicalIVRecipes (one is a PHI, the other is not)
updated, thanks


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

Exactly, I added. a comment.

> 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()?

VPWidenCanonicalIVRecipe should probably be placed outside the phis section, as it doesn't expand to phi instructions. I put up D116473. I don't think there's a strong need for more involved accessors like `real-phis` and `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
----------------
Ayal wrote:
> "// For first-order recurrences [, canonical IV] and in-order reduction phis ..."
> 
> "Otherwise" - for non ordered reductions,
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:178
   }
 
   for (const VPRegionBlock *Region :
----------------
Ayal wrote:
> 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.
> 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.
Sounds good, we can adjust it once this is modeled with recipes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113223