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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Two more notes, otherwise this looks good to me :) @lebedev.ri Anything to add?



================
Comment at: llvm/lib/IR/ConstantRange.cpp:834
+    if (isWrappedSet() || Other.isWrappedSet())
+      return add(Other);
+    APInt LMin = getUnsignedMin(), LMax = getUnsignedMax();
----------------
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.


================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:712
+    if (CR1.isWrappedSet() || CR2.isWrappedSet())
+      return;
+
----------------
Rather than skipping these entirely, you can still check that the result is correct, even if not minimal. Something like this:

```
ConstantRange CR = RangeFn(CR1, CR2);
//... Loop
if (!IsOverflow) {
  // ... Collect Min/Max
  EXPECT_TRUE(CR.contains(N));
}

if (!CR1.isWrappedSet() && !CR2.isWrappedSet()) {
    // ... Do the exact check here
}
```


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

https://reviews.llvm.org/D67339