[PATCH] D118642: [IVDescriptor] Find original 'Previous' for first-order recurrences.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:921
+    // Do not try to sink an instruction multiple times (if multiple operands
+    // are first order recurrences).
     auto It = SinkAfter.find(SinkCandidate);
----------------
In addition to "Do not", explain what we "Do", e.g.:
// Avoid sinking an instruction multiple times (if multiple operands are first order recurrences) by sinking once - after the latest 'previous' instruction.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:927
+      // the chain is the original 'Previous' for a recurrence handled earlier.
+      while (SinkAfter.count(OtherPrev))
+        OtherPrev = SinkAfter[OtherPrev];
----------------
This traversal is going upwards, from use to def, maintaining original dependences in order, right? Worth asserting that each SinkAfter[OtherPrev] comes before OtherPrev, and that the initial OtherPrev comes before the last OtherPrev?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:930
+
+      // SinkCandidate is already being sunk after a instruction after Previous.
+      // Nothing left to do.
----------------
a[n] instruction


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:935
+      // Otherwise, Previous comes after OtherPrev and SinkCandidate needs to be
+      // re-sunk to Previous.
     }
----------------
instead of sinking to OtherPrev.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118642