[ConstantRange] Simplify unittests after getSetSize was removed

[lebedev.ri]

Please just revert those two commits.

[nikic]

unittests/IR/ConstantRangeTest.cpp

59 ↗

(On Diff #195050)

It would seem more natural to write this as a do/while loop:

APInt N = CR.getLower();
do {
    TestFn(N);
} while (++N != CR.getUpper());

1468 ↗

(On Diff #195050)

Should use ForeachNumInConstantRange twice here (and drop the comment about sizes above).

[MaskRay]

The commit message was

The last use of this helper method was removed by rL302385.

Which is clearly not true, as this commit removed more uses of that method, that weren't testing the method itself, but using it. Therefore i'm questioning the whole removal. Moving it into a static function in tests is another, unrelated matter.

That commit didn't remove more uses of that method. The last library use was removed two years ago and no use has been added since then. I deleted it, and then realized there was a unittest relying on it.

and the way the test was rewritten to avoid it looks more complicated than it was with the function, at least to me..

Please just revert those two commits.

I agree that the last commit made it more complex than it should. I apologize for that. So I created this revision. However, "more complicated than it was with the function" is your opinion and I'm not convinced of that judgement. So, I'd like to get more input from others, especially those who removed the last few library uses.

[MaskRay]

simplify

[lebedev.ri]

In D60662#1465668, @MaskRay wrote:

The commit message was

The last use of this helper method was removed by rL302385.

Which is clearly not true, as this commit removed more uses of that method, that weren't testing the method itself, but using it. Therefore i'm questioning the whole removal. Moving it into a static function in tests is another, unrelated matter.

That commit didn't remove more uses of that method. The last library use was removed two years ago and no use has been added since then. I deleted it, and then realized there was a unittest relying on it.

That is precisely my point, it was still used, in test.
As you have just wrote, that wasn't considered when dropping the method.

and the way the test was rewritten to avoid it looks more complicated than it was with the function, at least to me..

Please just revert those two commits.

I agree that the last commit made it more complex than it should. I apologize for that. So I created this revision. However, "more complicated than it was with the function" is your opinion and I'm not convinced of that judgement. So, I'd like to get more input from others, especially those who removed the last few library uses.

[MaskRay]

In D60662#1465673, @lebedev.ri wrote:

In D60662#1465668, @MaskRay wrote:

The commit message was

The last use of this helper method was removed by rL302385.

Which is clearly not true, as this commit removed more uses of that method, that weren't testing the method itself, but using it. Therefore i'm questioning the whole removal. Moving it into a static function in tests is another, unrelated matter.

That commit didn't remove more uses of that method. The last library use was removed two years ago and no use has been added since then. I deleted it, and then realized there was a unittest relying on it.

That is precisely my point, it was still used, in test.
As you have just wrote, that wasn't considered when dropping the method.

When my first commit was made, it was indeed used in the unittest (I don't index /(clang|lld|llvm)/(test|unittests)/ with my language server so I missed the references in unittests, it seems I should), but I soon fixed that (the second commit). It wasn't elegant and I saw your complaint, so I created this revision.

and the way the test was rewritten to avoid it looks more complicated than it was with the function, at least to me..

Please just revert those two commits.

I agree that the last commit made it more complex than it should. I apologize for that. So I created this revision. However, "more complicated than it was with the function" is your opinion and I'm not convinced of that judgement. So, I'd like to get more input from others, especially those who removed the last few library uses.

[nikic]

LGTM. The end result here looks better to me than the original getSetSize() based code, so we'd want to do these changes even independently of that removal.

unittests/IR/ConstantRangeTest.cpp

1463–1464 ↗

(On Diff #195051)

The "These loops..." etc part of this comment should be dropped, as it's no longer relevant with the current implementation.

1468 ↗

(On Diff #195051)

I think we should drop these two asserts (or at least put them inside ForeachNumInConstantRange, but I don't think it's worthwhile to have them).

[MaskRay]

Apply nikic's suggestion

llvm/trunk/unittests/IR/ConstantRangeTest.cpp

EnumerateConstantRanges(Bits, [&](const ConstantRange &CR1) {
     EnumerateConstantRanges(Bits, [&](const ConstantRange &CR2) {
       TestFn(CR1, CR2);
     });
   });
 }
 
 template<typename Fn>
 static void ForeachNumInConstantRange(const ConstantRange &CR, Fn TestFn) {
-  if (CR.isFullSet()) {
-    for (APInt N = APInt::getNullValue(CR.getBitWidth());;) {
-      TestFn(N);
-      if (N == APInt::getAllOnesValue(CR.getBitWidth()))
-        break;
-      ++N;
-    }
-    return;
+  if (!CR.isEmptySet()) {
+    APInt N = CR.getLower();
+    do TestFn(N);
+    while (++N != CR.getUpper());
   }
-  for (APInt N = CR.getLower(); N != CR.getUpper(); ++N)
-    TestFn(N);
 }
 
 ConstantRange ConstantRangeTest::Full(16, true);
 ConstantRange ConstantRangeTest::Empty(16, false);
 ConstantRange ConstantRangeTest::One(APInt(16, 0xa));
 ConstantRange ConstantRangeTest::Some(APInt(16, 0xa), APInt(16, 0xaaa));
 ConstantRange ConstantRangeTest::Wrap(APInt(16, 0xaaa), APInt(16, 0xa));
 

 }
 
 template<typename Fn1, typename Fn2>
 static void TestOverflowExhaustive(Fn1 OverflowFn, Fn2 MayOverflowFn) {
   // Constant range overflow checks are tested exhaustively on 4-bit numbers.
   unsigned Bits = 4;
   EnumerateTwoConstantRanges(Bits, [=](const ConstantRange &CR1,
                                        const ConstantRange &CR2) {
-    unsigned Size1 = CR1.isFullSet()
-                         ? 1u << CR1.getBitWidth()
-                         : (CR1.getUpper() - CR1.getLower()).getZExtValue();
-    unsigned Size2 = CR2.isFullSet()
-                         ? 1u << CR2.getBitWidth()
-                         : (CR2.getUpper() - CR2.getLower()).getZExtValue();
-
     // Loop over all N1 in CR1 and N2 in CR2 and check whether any of the
-    // operations have overflow / have no overflow. These loops are based
-    // on Size1/Size2 to properly handle empty/full ranges.
+    // operations have overflow / have no overflow.
     bool RangeHasOverflow = false;
     bool RangeHasNoOverflow = false;
-    APInt N1 = CR1.getLower();
-    for (unsigned I1 = 0; I1 < Size1; ++I1, ++N1) {
-      APInt N2 = CR2.getLower();
-      for (unsigned I2 = 0; I2 < Size2; ++I2, ++N2) {
-        assert(CR1.contains(N1));
-        assert(CR2.contains(N2));
-
+    ForeachNumInConstantRange(CR1, [&](const APInt &N1) {
+      ForeachNumInConstantRange(CR2, [&](const APInt &N2) {
         if (OverflowFn(N1, N2))
           RangeHasOverflow = true;
         else
           RangeHasNoOverflow = true;
-      }
-    }
+      });
+    });
 
     ConstantRange::OverflowResult OR = MayOverflowFn(CR1, CR2);
     switch (OR) {
     case ConstantRange::OverflowResult::AlwaysOverflows:
       EXPECT_TRUE(RangeHasOverflow);
       EXPECT_FALSE(RangeHasNoOverflow);
       break;
     case ConstantRange::OverflowResult::NeverOverflows: