[PATCH] D13612: [IR] Add a `makeNoWrapRegion` method to `ConstantRange`

[Nick Lewycky via llvm-commits]

nlewycky added a comment.

The logic looks correct, and I really like the test!


================
Comment at: include/llvm/IR/ConstantRange.h:89-93
@@ +88,7 @@
+  /// Example:
+  ///  makeNoWrapRegion(Add, i8 1, AddOperator::NoSignedWrap) == [-128, 127)
+  ///  makeNoWrapRegion(Add, i8 1, AddOperator::NoUnsignedWrap) == [0, -1)
+  ///  makeNoWrapRegion(Add, i8 0, AddOperator::NoUnsignedWrap) == Full Set
+  static ConstantRange makeNoWrapRegion(Instruction::BinaryOps BinOp,
+                                        const APInt &C, bool SignedWrap);
+
----------------
Example in comments don't match function declaration? Did you want to make it an 'int WrappingFlags'? (I see that you actually use it as a bool, but I really like the named flags API shown in the comment much more.)

================
Comment at: lib/IR/ConstantRange.cpp:138
@@ +137,3 @@
+    // Conservative answer: empty set
+    return ConstantRange(C.getBitWidth(), false);
+
----------------
use BitWidth here

================
Comment at: lib/IR/ConstantRange.cpp:140
@@ +139,3 @@
+
+  if (C == 0)
+    // Full set: nothing signed / unsigned wraps when added to 0.
----------------
C.isMinValue() is fully defined in the header.

================
Comment at: lib/IR/ConstantRange.cpp:151
@@ +150,3 @@
+
+  assert(C.isNegative() && "Should be!");
+  return ConstantRange(APInt::getSignedMinValue(BitWidth) - C,
----------------
I think that's locally obvious enough that we don't need an assert. :)


http://reviews.llvm.org/D13612