[PATCH] D67339: [ConstantRange] add helper function addWithNoWrap

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:834
+    if (isWrappedSet() || Other.isWrappedSet())
+      return add(Other);
+    APInt LMin = getUnsignedMin(), LMax = getUnsignedMax();
----------------
shchenz wrote:
> nikic wrote:
> > You can remove the check for isWrappedSet / isSignWrappedSet now. We're going to intersect with the `add()` result anyway, so this is just going to intersect with itself. Even if one of the ranges is wrapped, we might still get a better result here if the other isn't. E.g. something like `[10, MAX] + Wrapped` will still get you `[10, MAX]` as the result here, which is likely better than what the wrapped calculation will provide.
> I am a little confuse about removing the check for isWrappedSet / isSignWrappedSet.
> Assuming we have two unsigned ranges [10, 20), [200, 100), if we remove the check for wrap, we get [10, 20) as result.
> 
> But if we do the math, [10, 20) +nuw [200, 100) results in two disjoint ranges [10, 119) \/ [210, 0).
> My previous is a little like `getPreferredRange`, we choose one range from above disjoint ranges based on preferred type.
> 
> If we use [10, 20), will it be too aggresive, since we use a smaller range?
> If we use `add` result for wrap range, we get [210, 119) as result which is father range of "[10, 119) \/ [210, 0)", it is conservative but right?
> Assuming we have two unsigned ranges [10, 20), [200, 100), if we remove the check for wrap, we get [10, 20) as result.

In this case, you should get `[10, 0)` as the result. The wrapped range will be treated as a full range by getUnsignedMin() and getUnsignedMax(), so this is the same as doing `[10, 20) + full`.

Additionally, as you say, you will get `[210, 119)` from the normal add. These are then intersected and you will get back either `[10, 0)` (if there is an unsigned preference) or `[210, 119)` (if there is a smallest range preference). For SCEV, getting `[10, 0)` back will be more useful than getting the wrapped range back.


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

https://reviews.llvm.org/D67339