[KnownBitsTest] Common up isCorrect and isOptimal. NFC.

[jayfoad]

This de-duplicates the code that prints useful information when a test
fails.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Jay Foad (jayfoad)

Changes

This de-duplicates the code that prints useful information when a test
fails.

---

Full diff: https://github.com/llvm/llvm-project/pull/89585.diff

1 Files Affected:

• (modified) llvm/unittests/Support/KnownBitsTest.cpp (+25-40)

diff --git a/llvm/unittests/Support/KnownBitsTest.cpp b/llvm/unittests/Support/KnownBitsTest.cpp
index feb0231300f1fd..2fa650c2c3bf0e 100644
--- a/llvm/unittests/Support/KnownBitsTest.cpp
+++ b/llvm/unittests/Support/KnownBitsTest.cpp
@@ -25,26 +25,20 @@ using BinaryBitsFn =
 using BinaryIntFn =
     llvm::function_ref<std::optional<APInt>(const APInt &, const APInt &)>;
 
-static testing::AssertionResult isCorrect(const KnownBits &Exact,
-                                          const KnownBits &Computed,
-                                          ArrayRef<KnownBits> Inputs) {
-  if (Computed.Zero.isSubsetOf(Exact.Zero) &&
-      Computed.One.isSubsetOf(Exact.One))
-    return testing::AssertionSuccess();
-
-  testing::AssertionResult Result = testing::AssertionFailure();
-  Result << "Inputs = ";
-  for (const KnownBits &Input : Inputs)
-    Result << Input << ", ";
-  Result << "Computed = " << Computed << ", Exact = " << Exact;
-  return Result;
-}
-
-static testing::AssertionResult isOptimal(const KnownBits &Exact,
-                                          const KnownBits &Computed,
-                                          ArrayRef<KnownBits> Inputs) {
-  if (Computed == Exact)
-    return testing::AssertionSuccess();
+static testing::AssertionResult checkResult(const KnownBits &Exact,
+                                            const KnownBits &Computed,
+                                            ArrayRef<KnownBits> Inputs,
+                                            bool CheckOptimality) {
+  if (CheckOptimality) {
+    // We generally don't want to return conflicting known bits, even if it is
+    // legal for always poison results.
+    if (Exact.hasConflict() || Computed == Exact)
+      return testing::AssertionSuccess();
+  } else {
+    if (Computed.Zero.isSubsetOf(Exact.Zero) &&
+        Computed.One.isSubsetOf(Exact.One))
+      return testing::AssertionSuccess();
+  }
 
   testing::AssertionResult Result = testing::AssertionFailure();
   Result << "Inputs = ";
@@ -71,12 +65,7 @@ static void testUnaryOpExhaustive(UnaryBitsFn BitsFn, UnaryIntFn IntFn,
       });
 
       EXPECT_TRUE(!Computed.hasConflict());
-      EXPECT_TRUE(isCorrect(Exact, Computed, Known));
-      // We generally don't want to return conflicting known bits, even if it is
-      // legal for always poison results.
-      if (CheckOptimality && !Exact.hasConflict()) {
-        EXPECT_TRUE(isOptimal(Exact, Computed, Known));
-      }
+      EXPECT_TRUE(checkResult(Exact, Computed, Known, CheckOptimality));
     });
   }
 }
@@ -102,12 +91,8 @@ static void testBinaryOpExhaustive(BinaryBitsFn BitsFn, BinaryIntFn IntFn,
         });
 
         EXPECT_TRUE(!Computed.hasConflict());
-        EXPECT_TRUE(isCorrect(Exact, Computed, {Known1, Known2}));
-        // We generally don't want to return conflicting known bits, even if it
-        // is legal for always poison results.
-        if (CheckOptimality && !Exact.hasConflict()) {
-          EXPECT_TRUE(isOptimal(Exact, Computed, {Known1, Known2}));
-        }
+        EXPECT_TRUE(
+            checkResult(Exact, Computed, {Known1, Known2}, CheckOptimality));
         // In some cases we choose to return zero if the result is always
         // poison.
         if (RefinePoisonToZero && Exact.hasConflict()) {
@@ -201,23 +186,23 @@ static void TestAddSubExhaustive(bool IsAdd) {
 
       KnownBits KnownComputed = KnownBits::computeForAddSub(
           IsAdd, /*NSW=*/false, /*NUW=*/false, Known1, Known2);
-      EXPECT_TRUE(isOptimal(Known, KnownComputed, {Known1, Known2}));
+      EXPECT_TRUE(checkResult(Known, KnownComputed, {Known1, Known2},
+                              /*CheckOptimality=*/true));
 
       KnownBits KnownNSWComputed = KnownBits::computeForAddSub(
           IsAdd, /*NSW=*/true, /*NUW=*/false, Known1, Known2);
-      if (!KnownNSW.hasConflict())
-        EXPECT_TRUE(isOptimal(KnownNSW, KnownNSWComputed, {Known1, Known2}));
+      EXPECT_TRUE(checkResult(KnownNSW, KnownNSWComputed, {Known1, Known2},
+                              /*CheckOptimality=*/true));
 
       KnownBits KnownNUWComputed = KnownBits::computeForAddSub(
           IsAdd, /*NSW=*/false, /*NUW=*/true, Known1, Known2);
-      if (!KnownNUW.hasConflict())
-        EXPECT_TRUE(isOptimal(KnownNUW, KnownNUWComputed, {Known1, Known2}));
+      EXPECT_TRUE(checkResult(KnownNUW, KnownNUWComputed, {Known1, Known2},
+                              /*CheckOptimality=*/true));
 
       KnownBits KnownNSWAndNUWComputed = KnownBits::computeForAddSub(
           IsAdd, /*NSW=*/true, /*NUW=*/true, Known1, Known2);
-      if (!KnownNSWAndNUW.hasConflict())
-        EXPECT_TRUE(isOptimal(KnownNSWAndNUW, KnownNSWAndNUWComputed,
-                              {Known1, Known2}));
+      EXPECT_TRUE(checkResult(KnownNSWAndNUW, KnownNSWAndNUWComputed,
+                              {Known1, Known2}, /*CheckOptimality=*/true));
     });
   });
 }

[jayfoad]

This if is now unnecessary since I moved the handling of always-poison results into checkResult. Same below.

[nikic]

LGTM

[RKSimon]

LGTM

[ilovepi]

we're getting an assertion failure with this patch. I bisected it to this patch. I'm including a reproducer (not reduced) and my bisect script.

Failing bot: https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.arm64-debug-subbuild/b8749763985778255185/overview
Logs: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8749760927129551041/+/u/clang/build/stdout

repro.zip

I bisected with

git bisect good a2692ac23f1421097f0d51c7325045ed38db6408b
git bisect bad d3f6a88a1fb36f94c71940514e576821c6cc3ade
git bisect run bisect.sh

Note if you want to use the bisect script you may need to update paths. The .cpp and other .sh are from the generated reproducer, so should work if you update the path to clang in the script.

I have creduce running now, so I can probably post a smaller reproducer in a while.

In the meantime, can you revert this until a fix is ready?

[ilovepi]

Actually, looking at your patch, I'm not sure how this is causing an assertion failure. Still, can you take a look?

[ilovepi]

oh, sorry for the noise. You can disregard my comments. seems like I started w/ a bad commit range in the bisect. So just to be clear, this patch is fine, and I'm very wrong.