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

[Philip Reames via Phabricator via llvm-commits]

reames 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());
----------------
anna wrote:
> mkazantsev wrote:
> > 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.
> > 
> thanks for the clarification! I have added comments on the other review thread about detecting empty ranges.
One possibility, you could assert that we don't know that End <= Begin.  As before, we might not be able to confirm that, but then at least if we do, we know we have a problem.

I suspect SCEV already has a function which takes a Predicate and two SCEVs, and returns an optional bool.  If it doesn't, adding one might be reasonable and easily extractable.

This would be a good stand alone change to land separately.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1810
+      if (IRC.isSigned() != LS.IsSignedPredicate &&
+          !RangeIsNonNegative(Result.getValue()))
+        continue;
----------------
This is correct, but a likely cleaner strategy overall would be normalize ranges at the point of construction.  If, for instance, all strictly positive ranges were always considered unsigned, then this check would disappear.  

Might be a good standalone cleanup change after this goes in.


https://reviews.llvm.org/D38581