[PATCH] D69918: [CR][WIP] Add `subWithNoWrap()` method

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D69918#1736111 <https://reviews.llvm.org/D69918#1736111>, @nikic wrote:

> @lebedev.ri I suspect this is due to the intersection with the raw `add()` range. Maybe it happens that the intersection between `add()` and `[MAX, MAX]` is always empty in the case where the overflow check triggers? Can you try removing that intersection and see what happens?


Ah, that appears to be the case:

  diff --git a/llvm/lib/IR/ConstantRange.cpp b/llvm/lib/IR/ConstantRange.cpp
  index 21a93b723b7..6e0c388aaa9 100644
  --- a/llvm/lib/IR/ConstantRange.cpp
  +++ b/llvm/lib/IR/ConstantRange.cpp
  @@ -882,8 +882,16 @@ ConstantRange ConstantRange::addWithNoWrap(const ConstantRange &Other,
   
     if (NoWrapKind & OBO::NoSignedWrap)
       Result = Result.intersectWith(addWithNoSignedWrap(Other), RangeType);
  -  if (NoWrapKind & OBO::NoUnsignedWrap)
  +  if (NoWrapKind & OBO::NoUnsignedWrap) {
       Result = Result.intersectWith(addWithNoUnsignedWrap(Other), RangeType);
  +
  +    APInt LMin = getUnsignedMin(), LMax = getUnsignedMax();
  +    APInt RMin = Other.getUnsignedMin(), RMax = Other.getUnsignedMax();
  +    bool Overflow;
  +    APInt NewMin = LMin.uadd_ov(RMin, Overflow);
  +    if(Overflow)
  +      assert(Result.isEmptySet());
  +  }
     return Result;
   }

Didn't check signed case.

> If that's the case, I'm not sure whether or not we should rely on that, it seems rather subtle.

Now that i've added `EXPECT_EQ(CR.isEmptySet(), AllOverflow);` test,
that seems like a safe assumption to me? Seems like a net win to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69918