[PATCH] D97219: [LSR] Unify scheduling of existing and inserted addrecs

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5597
+        !llvm::all_of(IncV->uses(),
+                      [&](Use &U) {return DT.dominates(IVIncInsertPos, U);}))
+      continue;
----------------
mkazantsev wrote:
> I think `DT.dominates(IncV, IVIncInsertPos)` is not what we should be checking to enforce legality. It's totally legal to move `IncV` before `IVIncInsertPos` if `DT.dominates(IVIncInsertPos, IncV)`. It's maybe not what we want performance-wise, but it's legal.
> 
> I think the real legality check needed here is `DT.dominates(IVIncInsertPos, Latch)`.
(previous response comment got lost?)

It turns out the whole check is unneeded.  What I actually needed to check was that all of the operands dominate the insert location, but that's already implied from the other checks.  We know the phi must dominate everything in the loop, and constants trivially dominate.


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

https://reviews.llvm.org/D97219