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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/IR/ConstantRange.h:338
+  ConstantRange addWithNoWrap(const ConstantRange &Other,
+                              unsigned NoWrapKind) const;
+
----------------
shchenz wrote:
> nikic wrote:
> > My preference would be to add this as an optional argument to `add()` instead. In the future, we'll likely want to accept NoWrapKind on more methods, and also thread it through binaryOp().
> Can I do it later in another seperated patch? I want to make this patch as small as possible. Combining with current `add` may introduce potential issue?
Sure, we can do that separately.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:833
+      (NoWrapKind == OBO::NoSignedWrap || NoWrapKind == OBO::NoUnsignedWrap) &&
+      "NoWrapKind invalid!");
+
----------------
shchenz wrote:
> nikic wrote:
> > I think we should also accept `OBO::NoSignedWrap|OBO::NoUnsignedWrap` here. This would allow directly passing through nowrap flags in LVI.
> > 
> > Possibly this can be implemented by computing both the nuw and the nsw result and intersecting both?
> I think there is no necessary for us to support `OBO::NoSignedWrap|OBO::NoUnsignedWrap` . We have different signed and unsigned ranges for one SCEV and we can only query one type range(Unsigned range or Signed range) for one time. Also we set nsw only based on signed range and nuw only based on unsigned range. Intersecting signed and unsigned range may get wrong result for SCEV's range if we use this intersecting range for signed/unsigned range. For example, if one ADDSCEV's signed range is [-10, 5), and its unsigned range is [10, 20), in StrengthenNoWrapFlags, we will set this SCEV with nsw and nuw. So we get Intersecting range is empty-set, if we use this range as signed range or unsigned range, we get empty-set, this should be wrong?
> I think there is no necessary for us to support OBO::NoSignedWrap|OBO::NoUnsignedWrap . We have different signed and unsigned ranges for one SCEV and we can only query one type range(Unsigned range or Signed range) for one time. Also we set nsw only based on signed range and nuw only based on unsigned range.

The addWithNoWrap() functionality is also useful outside of SCEV. LVI, which is used by CVP, does not distinguish between signed and unsigned ranges. It would simply pass through the nowrap flags on the (IR-level) add operation. It would be odd if analysis quality would decrease if an add had both nuw/nsw, rather than only one of them.

> Intersecting signed and unsigned range may get wrong result for SCEV's range if we use this intersecting range for signed/unsigned range. For example, if one ADDSCEV's signed range is [-10, 5), and its unsigned range is [10, 20), in StrengthenNoWrapFlags, we will set this SCEV with nsw and nuw. So we get Intersecting range is empty-set, if we use this range as signed range or unsigned range, we get empty-set, this should be wrong?

Getting an empty set is correct in this case. The unsigned and signed SCEV ranges are *both* (conseratively) correct and the real range will be a subset of their intersection.

In D64869, you're using the addNoWrap() functionality by only setting nuw if both the add is nuw and the preferred range type is unsigned (and same for signed). I think this makes sense as a conservative choice that avoids regressions, but generally these are really orthogonal: The nowrap flags and the preferred signedness are independent concepts and may be mixed.

I think the ideal design here would actually be to expose both as separate arguments, nowrap using OBO flags and signedness using ConstantRange::PreferredRangeType:

    ConstantRange addWithNoWrap(
        const ConstantRange &Other, unsigned NoWrapKind, PreferredRangeType RangeType);

In this case the implementation could, at a high level, look something like this:

```
// Normal wrapping addition as baseline.
ConstantRange Res = add(Other);
if (NoWrapKind & NUW) {
    // Intersect in NUW addition based on getUnsignedMin/Max().
    Res = Res.intersect(addNUW(Other), RangeType);
}
if (NoWrapKind == NSW) {
    // Intersect in NSW addition based on getSignedMin/Max().
    Res = Res.intersect(addNSW(Other), RangeType);
}
return Res;
```

I think this is both flexible in terms of how it may be used, and intuitive in how it works/behaves.

To take a specific example, consider an `add nuw nsw i4 [0, 3], [5, 7]`:

    add: [5, 10]
    addNUW: [5, 10]
    addNSW: [5, 7]
    intersection: [5, 7] (independent of sign preference in this case)

If this was a SCEV with an unsigned preference, then taking just the nuw flag would have given us a worse result than considering both. Even for the unsigned range, the nsw flag still provides useful information.


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

https://reviews.llvm.org/D67339