[PATCH] D142886: [VPlan] Switch to checking sinking legality for recurrences in VPlan.

[Ayal Zaks via Phabricator via llvm-commits]

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

LGTM



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:967
 
-  if (!Previous || !TheLoop->contains(Previous) || isa<PHINode>(Previous) ||
-      SinkAfter.count(Previous)) // Cannot rely on dominance due to motion.
+  if (!Previous || !TheLoop->contains(Previous) || isa<PHINode>(Previous))
     return false;
----------------
fhahn wrote:
> Ayal wrote:
> > (Relying here on Previous is fragile as it may have been considered for sinking.)
> Agreed, but I think until we fully check legality in VPlan I think it is needed for now here (and I think it is not different to the current handling)
(Above comment refers to ignoring if SinkAfter(Previous), which is the source of fragility/optimism.)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9020
+  if (!VPlanTransforms::adjustFixedOrderRecurrences(*Plan, Builder))
+    return std::nullopt;
 
----------------
TODO (Independent of this patch): Consider moving this potentially bailing-out check earlier to save compile-time for bailed-out cases, which would avoid vectorizing altogether.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:656
 // Sink users of \p FOR after the recipe defining the previous value \p Previous
 // of the recurrence.
+static bool
----------------
Add meaning of return value?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:666
   auto TryToPushSinkCandidate = [&](VPRecipeBase *SinkCandidate) {
-    assert(
-        SinkCandidate != Previous &&
-        "The previous value cannot depend on the users of the recurrence phi.");
+    // The previous value cannot depend on the users of the recurrence phi.
+    if (SinkCandidate == Previous)
----------------
"cannot" >> "must not"?
If SinkCandidate equals Previous a cycle is closed and phi is not a fixed order recurrence.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:740
+      return false;
+    SeenPrevious.insert(Previous);
 
----------------
fhahn wrote:
> Ayal wrote:
> > Is `SeenPrevious` really needed, to make sure chained FORs are processed in order, or can it be removed - given that every FOR takes care of its own sinkings followed by introducing its splice hooking it up to defs and uses? Potentially sinking candidates multiple times.
> I checked and it is not needed in the latest version indeed! It also yields a nice improvement in `@test_pr54223_sink_after_insertion_order`! Updated to remove it.
Very well! A direct consequence of VPlan's design to rely on updated def-use IR rather than recorded maps and sets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142886