[PATCH] D38581: [IRCE] Fix intersection between signed and unsigned ranges

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1753
+  auto RangeIsNonNegative = [&](InductiveRangeCheck::Range &R) {
+    return SE.isKnownNonNegative(R.getBegin()) &&
+           SE.isKnownNonNegative(R.getEnd());
----------------
mkazantsev wrote:
> anna wrote:
> > Is this enough for proving range is non-negative?
> > 
> > Looking at this lambda alone with no context here, we can have [5, 2] and still have range wrap around.
> > Is there a way to add an assert that getBegin < getEnd?
> In `parseLoopStructure` we assert that the IV `HasNoSignedWrap`. Interesting detail is that currently it only checks for `nsw`. My guess is that we should check `nuw` for unsigned IVs instead. I will check with that and possibly add some tests.
> 
> But generally, we assume that the IV does not have wraparound, and ranges for range checks are also in canonical form from a smaller value to a greater value.
Thanks for pointing this situation out! Indeed, it wasn't handled before. As result, sometimes IRCE applied to safe ranges like `[5, 2)` ending up with making completely unprofitable transformations.

We cannot assert that `Begin < End` because in the most common situation we don't know it (for example, iteration from 0 to `N` where `N` is non-negative is potentially an empty loop, as well as iteration from `A` to `B`; we might be not able to say anything from SCEVs, however if we knew that we iterated from `A` to `B` with `nsw` flag, that would give us this piece of information). So this check is over-conservative. 

Rather than asserting that `Begin < End` which is not always known without context (about the indvar and so on), I've prepared a patch https://reviews.llvm.org/D39082 that trats ranges for which we know that `Begin >= End` as empty.



https://reviews.llvm.org/D38581