[KnownBits] Add force{Non}Negative API

[goldsteinn]

These modification patterns are desired enough to justify having an
API for them.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: None (goldsteinn)

Changes

These modification patterns are desired enough to justify having an
API for them.

---

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

2 Files Affected:

• (modified) llvm/include/llvm/Support/KnownBits.h (+14)
• (modified) llvm/lib/Support/KnownBits.cpp (+11-21)

diff --git a/llvm/include/llvm/Support/KnownBits.h b/llvm/include/llvm/Support/KnownBits.h
index e4ec202f36aae0..751f7759fb2f20 100644
--- a/llvm/include/llvm/Support/KnownBits.h
+++ b/llvm/include/llvm/Support/KnownBits.h
@@ -114,6 +114,20 @@ struct KnownBits {
     Zero.setSignBit();
   }
 
+  /// Force this value to be negative. Unlike `makeNegative`, this will clear
+  /// any existing signbit.
+  void forceNegative() {
+    Zero.clearSignBit();
+    One.setSignBit();
+  }
+
+  /// Force this value to be non-negative. Unlike `makeNonNegative`, this will
+  /// clear any existing signbit.
+  void forceNonNegative() {
+    One.clearSignBit();
+    Zero.setSignBit();
+  }
+
   /// Return the minimal unsigned value possible given these KnownBits.
   APInt getMinValue() const {
     // Assume that all bits that aren't known-ones are zeros.
diff --git a/llvm/lib/Support/KnownBits.cpp b/llvm/lib/Support/KnownBits.cpp
index 6863c5c0af5dca..9ae912d64d0a1e 100644
--- a/llvm/lib/Support/KnownBits.cpp
+++ b/llvm/lib/Support/KnownBits.cpp
@@ -578,8 +578,7 @@ KnownBits KnownBits::abs(bool IntMinIsPoison) const {
     // is poison anyways.
     if (IntMinIsPoison && Tmp.countMinPopulation() == 1 &&
         Tmp.countMaxPopulation() != 1) {
-      Tmp.One.clearSignBit();
-      Tmp.Zero.setSignBit();
+      Tmp.forceNonNegative();
       KnownAbs.One.setBits(getBitWidth() - Tmp.countMinLeadingZeros(),
                            getBitWidth() - 1);
     }
@@ -596,10 +595,8 @@ KnownBits KnownBits::abs(bool IntMinIsPoison) const {
     // We only know that the absolute values's MSB will be zero if INT_MIN is
     // poison, or there is a set bit that isn't the sign bit (otherwise it could
     // be INT_MIN).
-    if (IntMinIsPoison || (!One.isZero() && !One.isMinSignedValue())) {
-      KnownAbs.One.clearSignBit();
-      KnownAbs.Zero.setSignBit();
-    }
+    if (IntMinIsPoison || (!One.isZero() && !One.isMinSignedValue()))
+      KnownAbs.forceNonNegative();
   }
 
   return KnownAbs;
@@ -658,26 +655,19 @@ static KnownBits computeForSatAddSub(bool Add, bool Signed,
 
   if (Signed) {
     if (Add) {
-      if (LHS.isNonNegative() && RHS.isNonNegative()) {
+      if (LHS.isNonNegative() && RHS.isNonNegative())
         // Pos + Pos -> Pos
-        Res.One.clearSignBit();
-        Res.Zero.setSignBit();
-      }
-      if (LHS.isNegative() && RHS.isNegative()) {
+        Res.forceNonNegative();
+      if (LHS.isNegative() && RHS.isNegative())
         // Neg + Neg -> Neg
-        Res.One.setSignBit();
-        Res.Zero.clearSignBit();
-      }
+        Res.forceNegative();
     } else {
-      if (LHS.isNegative() && RHS.isNonNegative()) {
+      if (LHS.isNegative() && RHS.isNonNegative())
         // Neg - Pos -> Neg
-        Res.One.setSignBit();
-        Res.Zero.clearSignBit();
-      } else if (LHS.isNonNegative() && RHS.isNegative()) {
+        Res.forceNegative();
+      else if (LHS.isNonNegative() && RHS.isNegative())
         // Pos - Neg -> Pos
-        Res.One.clearSignBit();
-        Res.Zero.setSignBit();
-      }
+        Res.forceNonNegative();
     }
   } else {
     // Add: Leading ones of either operand are preserved.

[goldsteinn]

Im fairly certain the failure in buildkite is unrelated.

[nikic]

Historically, we've mostly used the clear + set pattern to avoid conflict assertions. These assertions no longer exists -- arguably, it is a good outcome if we get a conflict, because we could (but currently don't) fold to poison.

The API may still be useful in cases where you're not combining two unrelated facts but rather fixing up an incorrect value. But I don't think that's the usage you have in mind?

Overall, I think we should be changing these uses to plain makeNegative/makeNonNegative, if possible.

[goldsteinn]

Historically, we've mostly used the clear + set pattern to avoid conflict assertions. These assertions no longer exists -- arguably, it is a good outcome if we get a conflict, because we could (but currently don't) fold to poison.

The API may still be useful in cases where you're not combining two unrelated facts but rather fixing up an incorrect value. But I don't think that's the usage you have in mind?

Overall, I think we should be changing these uses to plain makeNegative/makeNonNegative, if possible.

Since we can handle conflicts there are definetly some places where make can but it previously couldn't work, but there are still uses for force. The case in computeForSat... still would need force and both intended cases in #110329 need force. But either way, if people don't think this will be useful its not important to get in.