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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the report! I recommitted the change with a fix for the compile-time issue in 6b8d19d2b57ecd45eb2ff1964798f3cd8c907ac4 <https://reviews.llvm.org/rG6b8d19d2b57ecd45eb2ff1964798f3cd8c907ac4>.  The new version of the patch adds a set to avoid duplicating work in isFixedOrderRecurrence, which was previously done through the removed SinkAfter map.



================
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
----------------
Ayal wrote:
> Add meaning of return value?
Thanks, documented!


================
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)
----------------
Ayal wrote:
> "cannot" >> "must not"?
> If SinkCandidate equals Previous a cycle is closed and phi is not a fixed order recurrence.
Adjusted the comment, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142886