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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Sorry for the delay. Ready for further review.



================
Comment at: llvm/include/llvm/IR/ConstantRange.h:338
+  ConstantRange addWithNoWrap(const ConstantRange &Other,
+                              unsigned NoWrapKind) const;
+
----------------
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?


================
Comment at: llvm/lib/IR/ConstantRange.cpp:833
+      (NoWrapKind == OBO::NoSignedWrap || NoWrapKind == OBO::NoUnsignedWrap) &&
+      "NoWrapKind invalid!");
+
----------------
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?


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

https://reviews.llvm.org/D67339