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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

The implementation looks correct to me. As @lebedev.ri mentioned we usually add exhaustive tests for ConstantRange methods nowadays. I'm not totally sure what should be tested here... I guess we should test a) conservative correctness (no wrong results produced) and b) tight bounds if the input sets are non-wrapping/non-sign-wrapping.



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


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


================
Comment at: llvm/lib/IR/ConstantRange.cpp:844
+    if (Overflow)
+      return ConstantRange::getEmpty(LMin.getBitWidth());
+    APInt NewMax = LMax.uadd_sat(RMax);
----------------
As this is a method on ConstantRange you can just write `return getEmpty();` here.


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

https://reviews.llvm.org/D67339