[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 inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8494
     VPHeaderPHIRecipe *PhiRecipe = nullptr;
-    assert((Legal->isReductionVariable(Phi) ||
-            Legal->isFirstOrderRecurrence(Phi)) &&
-           "can only widen reductions and first-order recurrences here");
-    VPValue *StartV = Operands[0];
-    if (Legal->isReductionVariable(Phi)) {
-      const RecurrenceDescriptor &RdxDesc =
-          Legal->getReductionVars().find(Phi)->second;
-      assert(RdxDesc.getRecurrenceStartValue() ==
-             Phi->getIncomingValueForBlock(OrigLoop->getLoopPreheader()));
-      PhiRecipe = new VPReductionPHIRecipe(Phi, RdxDesc, *StartV,
-                                           CM.isInLoopReduction(Phi),
-                                           CM.useOrderedReductions(RdxDesc));
-    } else {
-      PhiRecipe = new VPFirstOrderRecurrencePHIRecipe(Phi, *StartV);
-    }
+    if (Legal->isReductionVariable(Phi) || Legal->isFirstOrderRecurrence(Phi)) {
+      VPValue *StartV = Operands[0];
----------------
Ayal wrote:
> Should the assert be retained - what happens with a header `Phi` that is not an induction, reduction or FOR?
Yes it should be retained, I restored the original assert.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119661