[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#3690631 <https://reviews.llvm.org/D119661#3690631>, @fhahn wrote:

> Ping.
>
> Rebased the patch and also updated it to remember any header phis. Corresponding tests have been added in ff5ae948a7230 <https://reviews.llvm.org/rGff5ae948a72307fcd6b3be9ddf2d3b9349e02caf> & 6e1ba62d0dd <https://reviews.llvm.org/rG6e1ba62d0dd27686aa7779a122e04ac5abc6cecf>. I have also added additional runtime testing to llvm-test-suite in rT1846f600f7db <https://reviews.llvm.org/rT1846f600f7dbd41e82ee7961ab57556a0240bb25>.
>
> Note that the current handling is geared towards chains for first order recurrences. This means we widen each first-order recurrence separately and use the incoming value of the previous recurrence in the chain for the current one.
>
> This should be different to modeling a whole chain as a 2nd or higher order recurrence, for which we may be able to generate a single recurrence phi for the whole chain. The approach in the current patch focuses on the case where each phi in the chain has other users, so we would need to generate recurrence phis for each phi in the chain anyways. Dedicated higher-order recurrence support can be added as follow-up, but I expect the gains in practice to be limited.

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.

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.



================
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];
----------------
Should the assert be retained - what happens with a header `Phi` that is not an induction, reduction or FOR?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119661