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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

In D119661#3730086 <https://reviews.llvm.org/D119661#3730086>, @Ayal wrote:

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

Thanks, I'll update the description in the committed version.



================
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();
----------------
Ayal wrote:
> Should this be checking if the non-phi previous dominates all users of the recurrence?
I think the current code handles this, because the non-phi incoming value will be part of a fixed-order recurrence we are checking here. 


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


================
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?

I just realized that it looks like I never submitted this comment. I think the assertion doesn't add much, so I removed it in the committed version. Below my original response, if it is worth adding the assertion that could be done as follow up :) 

The main goal is to provide extra restrictions here. When creating header phis, the only incoming values that should be possible are header phis. All other recipes can only be recorded/added later. It might be overcautious and I could remove it as well.

> Good catch re: classof.

Moved the change with extra verification to D131989


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119661