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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5578
+        dyn_cast<Instruction>(PN.getIncomingValueForBlock(L->getLoopLatch()));
+    if (!IncV || (IncV->getOpcode() != Instruction::Add &&
+                  IncV->getOpcode() != Instruction::Sub))
----------------
nit: let's avoid `||` by splitting this into two checks.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5583
+    unsigned PhiOp = IncV->getOperand(0) == &PN ? 0 : 1;
+    if (IncV->getOperand(PhiOp) != &PN)
+      continue;
----------------
I'm pretty sure that at this point having constant as 2nd operand is the canonical form. Do we really care about `add 2, phi` case? If not, I'd propose `match (m_Add(m_Specific(PN), m_ConstantInt(Step)))` (and same for sub - if we care, because subs are also canonicalized to adds).


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5597
+        !llvm::all_of(IncV->uses(),
+                      [&](Use &U) {return DT.dominates(IVIncInsertPos, U);}))
+      continue;
----------------
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)`.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/post-increment-insertion.ll:140
+
+define i32 @test_04(i32* %p, i64 %len, i32 %x) {
+; CHECK-LABEL: @test_04(
----------------
Please commit these tests separately so the diff from this patch is obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97219