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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In D119661#3723058 <https://reviews.llvm.org/D119661#3723058>, @fhahn wrote:

> In D119661#3722786 <https://reviews.llvm.org/D119661#3722786>, @Ayal wrote:
>
>> Nice reuse of chained FOR's!
>> Conceptually by looking past previous header phi's, Legal->isFirstOrderRecurrence(Phi) may be more accurately renamed Legal->isFixedOrderRecurrence(Phi) - also potentially recording its associated non-header-phi "Previous", even if the recipe implements this pattern using multiple single-element FOR splices.
>
> Thanks for taking a look!
>
> I think renaming would require to also update the code in IVDescriptors which does the actual recurrence identification. Please let me know if you would like me to rename it accross LVLegality and IVDescriptors.

Yeah, it seems `First` is no longer accurate for both LVLegality and IVDescriptors (but is for the recipes). Is there any benefit in caching the non-header-phi `Previous` in the IVDescriptor, given the iteration-by-iteration implementation?

>> Wonder if some instCombine pattern may subsequently fold the multiple shuffles (and the multiple incoming vectors each holding a single element) to optimize higher-order recurrence where some phi's feed only other phi's w/o other users.
>
> Unfortunately it doesn't look like instcombine will be able to fold those shuffles, e.g.: https://llvm.godbolt.org/z/a8YoM6ev9

Worth leaving a TODO somewhere?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9000
+               dyn_cast<VPFirstOrderRecurrencePHIRecipe>(PrevRecipe))
+      PrevRecipe = PrevPhi->getBackedgeRecipe();
     VPBasicBlock *InsertBlock = PrevRecipe->getParent();
----------------
Note that FOR's do not form cyclic dependences (unlike inductions and reduction), hence this loop must terminate.


================
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");
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119661