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

[Anna Thomas via Phabricator via llvm-commits]

anna requested changes to this revision.
anna added a comment.
This revision now requires changes to proceed.

Some clarification questions and comments inline.



================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1753
+  auto RangeIsNonNegative = [&](InductiveRangeCheck::Range &R) {
+    return SE.isKnownNonNegative(R.getBegin()) &&
+           SE.isKnownNonNegative(R.getEnd());
----------------
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.


================
Comment at: test/Transforms/IRCE/unsigned_comparisons_ult.ll:345
+  store i32 0, i32* %addr
+  %next = icmp ult i32 %idx.next, -100
+  br i1 %next, label %loop, label %exit
----------------
Am I right that because of the ult for the compare, -100 is actually UINT_MAX - 99, i.e. the 2's complement of the number. 
So, the iteration space is within [0, SINT_MAX].


================
Comment at: test/Transforms/IRCE/unsigned_comparisons_ult.ll:360
+
+; CHECK:      test_09
+
----------------
Please add checks here as `CHECK-NOT` for pre and post loops.


https://reviews.llvm.org/D38581