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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn 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);
----------------
Ayal wrote:
> 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.
Thanks, extended the comment!


================
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];
----------------
Ayal wrote:
> 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?
Good point, I added the assertion in the loop. Note that for the last entry the comes-before relation won't hold. I also changed to loop to avoid using `SinkAfter[]`.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118642