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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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() &&
----------------
Ayal wrote:
> canSink >> canSinkAfter ?
> 
> auto >> bool ?
I used auto for the lambda, but I've got rid of the lambda for now and moved it down.


================
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)
----------------
Ayal wrote:
> 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.
Agreed. I guess to be sure we would have to recursively check all users, until we either reach a cycle or are outside the loop, right? I've added the (simple) check here because this is very easy to run into in practice. Would it be OK to have the assertion as follow-up?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69228