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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D98222#2621535 <https://reviews.llvm.org/D98222#2621535>, @mkazantsev wrote:

> Regarding the helper you are using: looking into `matchSimpleRecurrence`, I don't really think it's doing what it seems to be doing. Consider case:
>
>   loop:
>     iv = phi [start][iv.next]
>     x = load volatile ...  // Loop-variant load
>     iv.next = shl iv, x
>
> I didn't really run a test, but from the code I cannot figure why it wouldn't be recognized as a simple recurrence. I guess the loop-invariance check is missing inside this helper, not in your current patch. The logic there could also be simplified if we gave it LoopInfo with notion of latch (we wouldn't need 2 iterations in loop there). Please consider this refactoring.

The lack of an invariant check in the matching routine is deliberate.  Please see usage .e.g. in computeKnownBits.  Not all callers want the restriction on step being loop invariant.

In this particular case, you are correct that there is a missing invariance check.  I'd originally written this for constant step (which are trivially invariant), but forgot to add back the invariance check when generalizing.  Thanks for the catch!


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

https://reviews.llvm.org/D98222