[PATCH] D71690: [SCEV] get a more accurate range for AddRecExpr with nuw flag

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5661
+        MinValue = getUnsignedRangeMin(AddRec->getStart());
+      else {
+        ConstantRange StartRange =
----------------
shchenz wrote:
> nikic wrote:
> > shchenz wrote:
> > > nikic wrote:
> > > > shchenz wrote:
> > > > > nikic wrote:
> > > > > > shchenz wrote:
> > > > > > > sanjoy.google wrote:
> > > > > > > > Can you comment on why you need the else block?
> > > > > > > If we use `getUnsignedRangeMin(AddRec->getStart())` as minimum value for SignedRange of the start of AddRec, we may get wrong result. For example, assuming we get range `[137, 227)` as result of `getUnsignedRangeMin(AddRec->getStart());` for type `i8`,  if we want to return it as SignedRange, it will be `[-119, -29)`, this is invalid range for AddRec with `nuw` even for SignedRange?
> > > > > > Why is `[-119, -29)` an invalid range?
> > > > > For SCEV1 `{x,+,stride}<nuw>`, when we try to get a signed range for SCEV1, the result range should be contained in [0, SINT_MAX +1)? 
> > > > > If we use `getUnsignedRangeMin(AddRec->getStart())` to get start value range, it should be any value in [0, UINT_MAX + 1), so the start min can be a value between [ SINT_MAX +1, UINT_MAX + 1), like 137 for type `i8`. If the signed range for SCEV1 is [137, 227), it is not inside the expected range [0, 128). For this case, we should return empty for SCEV1 SignedRange?
> > > > I still don't really get it. Why would the range need to be inside [0, SINT_MAX]? For example, let's say the start range is `[-100, -90]`, then that could still result in a valid nuw addrec, e.g. because we have `iv == -10` as a loop exit condition. "nuw" doesn't imply that the numbers can't be "negative", as long as the addrec does not cross zero.
> > > Ah, thanks, got your idea.
> > > 
> > > we get
> > > `-->  {-10,+,1}<nuw><%loop> U: [-10,0) S: [-10,0)`
> > > 
> > > So signed range for a SCEV with `nuw` flag may still be a range in the negative part.
> > Thanks, this looks better. What isn't clear to me though is why we can't just always use `getUnsignedRangeMin()` here. Shouldn't that always be correct and potentially give a better result?
> > 
> > Or is there some rule that you are not allowed to use the unsigned range while computing a signed range?
> Thanks for your comment. @nikic I am not sure about using `getUnsignedRangeMin()` as signed range min value is right. For my understanding, signed range and unsigned range are different ranges, why we can use unsigned range min value as signed range min value? `nuw` can ensure this?
Maybe @sanjoy.google can comment on that? My understanding is that both the unsigned and signed ranges are valid and we are free to use either one, so using the unsigned one is more beneficial in an unsigned context. But I'm not totally sure if my understanding here is right.

In any case, even when not always using the unsigned range (maybe better to avoid unless we know for sure), you should still be able to simplify this code to:

```
ConstantRange StartRange = getRangeRef(Start, SignHint);
APInt MinValue = StartRange.getUnsignedMin();
```

This will return zero if the range crosses zero. There is no actual requirement here that the range is all-negative or all non-negative here, you just always need to pick the unsigned minimum of the range. This will usually be zero if the range is not all-negative or all non-negative, but there are exceptions. For example in your `[11, SINT_MIN + 1)` example from the test below the range can have both signs, but the unsigned min is still `11`.


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

https://reviews.llvm.org/D71690