[PATCH] D98222: [SCEV] Use trip count information to improve shift recurrence ranges

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5655
+  if (!L || L->getHeader() != P->getParent())
+    return CR;
+
----------------
mkazantsev wrote:
> `LI.isLoopHeader(P->getParent())`?
> I also wonder why is that needed. The implementation of `matchSimpleRecurrence` cannot recognize something which is not a header phi, because there is no other way to have BO as operand of Phi and vice versa at the same time. I guess this may be turned into assert after `matchSimpleRecurrence`.
You're right, this can become an assert.  It made since before some refactoring I did to the code before commit.   Not so much anymore.  Will fix.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5663
+  // TODO: Handle ashr and lshr cases to increase minimum value reported
+  if (BO->getOpcode() != Instruction::Shl || BO->getOperand(0) != P)
+    return CR;
----------------
mkazantsev wrote:
> Looks like it's better split it into two checks, as `ashr` and `lshr` will still need `BO->getOperand(0) != P`.
I'd prefer to leave this to when someone actually generalizes for lshr and ashr.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5665
+    return CR;
+
+  unsigned TC = getSmallConstantMaxTripCount(L);
----------------
mkazantsev wrote:
> Are we missing a check that another operand is loop-invariant, or it is implied by something?
We are, good catch.


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

https://reviews.llvm.org/D98222