[PATCH] D48399: [ConstantRange] Add support of mul in makeGuaranteedNoWrapRegion.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

LGTM modulo comments on the test coverage inline.

I didn't check the math too carefully since the test coverage looks good.



================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:1024
 
+TEST(ConstantRange, MakeGuaranteedNoWrapRegionMulSingleValue) {
+  typedef OverflowingBinaryOperator OBO;
----------------
All of these tests would have passed without your change right (since earlier we'd return an empty range for multiplication)?

If yes, maybe add a couple of tests that check that we return non-trivial results (i.e. not empty sets) for some inputs?


================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:1051
+      bool Overflow;
+      (void)APInt(8, I, true).smul_ov(APInt(8, V, true), Overflow);
+      EXPECT_EQ(!Overflow, Range.contains(APInt(8, V, true)));
----------------
Maybe add an `/*isSigned=*/true` here?


================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:1056
+
+  for (uint64_t I = std::numeric_limits<uint8_t>::min();
+       I <= std::numeric_limits<uint8_t>::max(); I++) {
----------------
Minor nit, but it looks like these three loops can be different test cases with more descriptive names.


https://reviews.llvm.org/D48399