[PATCH] D69228: [LV] Generalize conditions for sinking instrs for first order recurrences.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:703
   // TODO: Consider extending this sinking to handle other kinds of instructions
   // and expressions, beyond sinking a single cast past Previous.
+
----------------
Above TODO comment should be updated as well (thanks :-). It's possible to further extend sinking loads/stores as noted, which will affect interleave-group analysis; and also to sinking multiple users of Phi, which will need to preserve relative order to maintain dependences between themselves.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:706
+  // We can sink any instruction without side effects, as long as all users are
+  // dominated by the instructions we are sinking after.
+  auto canSink = [DT](Instruction *I, Instruction *SinkPoint) {
----------------
by the instruction[s]


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:707
+  // dominated by the instructions we are sinking after.
+  auto canSink = [DT](Instruction *I, Instruction *SinkPoint) {
+    return !I->mayHaveSideEffects() &&
----------------
canSink >> canSinkAfter ?

auto >> bool ?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:718
+    // If the user of the PHI is also the incoming value, we potentially have a
+    // reduction and there's nothing to sink.
+    if (Previous == I)
----------------
That's true, but it isn't checked below if Phi has more than one use, and it isn't checked in canSink (if any of I's uses is equal to SinkPoint). Perhaps it should be an assert, in all these places, verifying the precondition that Phi does not belong to any reduction cycle.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:723
+    if (I->getParent() == Phi->getParent() && canSink(I, Previous)) {
       if (!DT->dominates(Previous, I)) // Otherwise we're good w/o sinking.
         SinkAfter[I] = Previous;
----------------
Maybe better to first check if Previous already dominates I, before checking all of I's uses.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:733
         return false;
     }
 
----------------
The above for loop looks similar to the canSink[After]() lambda; maybe rename it allUsesDominatedBy() or something, and reapply here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69228