[PATCH] D119661: [LV] Support chained phis as incoming values for first-order recurs.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This LGTM, thanks!
Subject may also reflect that this patch extends First to Fixed.
Adding minor comments.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:889
   // if we have to sink such an instruction for another recurrence, as the
   // dominance requirement may not hold after sinking.
   BasicBlock *LoopLatch = TheLoop->getLoopLatch();
----------------
Should this be checking if the non-phi previous dominates all users of the recurrence?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8522
+            "If a recipe has already been recorded, it must be a "
+            "first-order recurrence, induction, or reduction phi");
+
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > I.e., is a VPHeaderPHIRecipe? Have a verifier assert that all header phi's are assigned VPHeaderPHIRecipes if desired instead of complicating the logic here?
> > I updated the code to just check `isa<VPHeaderPHIRecipe>`. This also surfaced the fact that `VPHeaderPHIRecipe::classof` was missing checks for `VPWidenPointerInduction`. This is also fixed.
> > 
> > I kept the assertion for now, as this allows us to verify the property of the `Ingredient2Recipe` mapping.
> Good catch re: classof.
> 
> Wonder about this property we're verifying here - the IR is traversed top-down and is currently visiting the loop header phi's, so any `Inc` across the backedge which is already recorded must be a header phi (verify that?), which in turn should be mapped to a header phi recipe?
Still unclear why this assert is important here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119661