[PATCH] D121617: [LV] Move code to place induction increment to VPlan post-processing.

Ayal Zaks via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Mar 27 05:24:00 PDT 2022


Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:990
 
+    // Set the correct incoming block for backedge values and move induction to
+    // latch.
----------------
fhahn wrote:
> Ayal wrote:
> > Could the incoming value to an induction be added similar to adding such values for reductions and FORs below, which have distinct values across the backedge? I.e., another case of SinglePartNeeded? Could PhiR->getBackedgeValue() work - if the induction PhiR is added as an (second) operand of itself?
> > 
> > Moving the last induction step to the end of the latch could then be done separately.
> > Could the incoming value to an induction be added similar to adding such values for reductions and FORs below, which have distinct values across the backedge? I.e., another case of SinglePartNeeded? Could PhiR->getBackedgeValue() work - if the induction PhiR is added as an (second) operand of itself?
> 
> Unfortunately I don't think so, without further changes. The recipe for the phi is currently mapped to the vector IV values for each part. The increment is the last part + VF. So we either would have to turn VPWidenIntOrFpInductionRecipe into a  multi-def or store the LLVM IR value in the recipe directly. Both seem like bigger workarounds in my opinion.
(That depends on one's definition of "workaround" ;-)

It would be good if the modeling in VPlan of inductions and header phi's is more consistent. This could perhaps be done later, in subsequent patches. Trying to capture the issue and potential resolutions:

VPHeaderPHIRecipe offers an interface for recording the backedge value as an operand, used by the Canonical IV and other header phi recipes which (must) have distinct backedge VPValues. Their modeling in VPlan thus matches the IR they generate. During VPlan execution, phi's are first generated with start values only, when traversing from header to latch, and are later revisited to add their backedge incoming values after the latch was generated.

VPWidenIntOrFpInductionRecipe is currently not a VPHeaderPHIRecipe, it does not support an explicit backedge value. VPWidenIntOrFpInductionRecipe currently represents both the header phi - explicitly (with UF per-part bumps) and its bump - implicitly; it is conceptually a multi-def recipe, but uses its bump only to feed itself, w/o exposing it to any other user. This implicit bump must wait until the latch is generated in order to be fully rewired to the header phi, but can be partially/incorrectly rewired to the header phi temporarily.

VPCanonicalIVPHIRecipe is currently a VPHeaderPHIRecipe; it represents only the header phi, having a separate CanonicalIVIncrement recipe designated to represent its bump, which feeds both VPCanonicalIVPHIRecipe and BranchOnCount; the canonical IV admittedly has an easier bump to generate than int/fp/pointer inductions. Both header phi and bump can alternatively be represented by a single multi-def recipe, as noted for VPWidenIntOrFpInductionRecipe.

Should VPWidenIntOrFpInductionRecipe expose the bump that feeds it across the backedge, for other potential users, and for transparency/consistency?
Should VPWidenIntOrFpInductionRecipe become a VPHeaderPHIRecipe, by supporting a backedge VPValue?
Should the rewiring of header phi's be delegated to VPHeaderPHIRecipe, say by adding a second 'execute' method to it, to be called after the latch is constructed, here or at the end of Region::execute?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121617



More information about the llvm-commits mailing list