[llvm] [APInt] Add default-disabled assertion to APInt constructor (PR #106524)

[Nikita Popov via llvm-commits]

https://github.com/nikic created https://github.com/llvm/llvm-project/pull/106524

If the uint64_t constructor is used, assert that the value is actually a signed or unsigned N-bit integer depending on whether the isSigned flag is set. Provide an implicitTrunc flag to restore the previous behavior, where the argument is silently truncated instead.

In this commit, implicitTrunc is enabled by default, which means that the new assertions are disabled and no actual change in behavior occurs. The plan is to flip the default once all places violating the assertion have been fixed. See #80309 for the scope of the necessary changes.

The primary motivation for this change is to avoid incorrectly specified isSigned flags. A recurring problem we have is that people write something like `APInt(BW, -1)` and this works perfectly fine -- until the code path is hit with `BW > 64`. Most of our i128 specific miscompilations are caused by variants of this issue.

The cost of the change is that we have to specify the correct isSigned flag (and make sure there are no excess bits) for uses where BW is always <= 64 as well.

>From 623ccc65afee96beb67ad0941b6090890fcba214 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Thu, 29 Aug 2024 12:15:38 +0200
Subject: [PATCH] [APInt] Add default-disabled assertion to APInt constructor

If the uint64_t constructor is used, assert that the value is
actually a signed or unsigned N-bit integer depending on whether
the isSigned flag is set. Provide an implicitTrunc flag to
restore the previous behavior, where the argument is silently
truncated instead.

In this commit, implicitTrunc is enabled by default, which means
that the new assertions are disabled and no actual change in
behavior occurs. The plan is to flip the default once all places
violating the assertion have been fixed. See #80309 for the scope
of the necessary changes.

The primary motivation for this change is to avoid incorrectly
specified isSigned flags. A recurring problem we have is that
people write something like `APInt(BW, -1)` and this works
perfectly fine -- until the code path is hit with `BW > 64`.
Most of our i128 specific miscompilations are caused by variants
of this issue.

The cost of the change is that we have to specify the correct
isSigned flag (and make sure there are no excess bits) for uses
where BW is always <= 64 as well.
---
 llvm/include/llvm/ADT/APInt.h    | 19 +++++++++++++++++--
 llvm/lib/Support/APInt.cpp       | 14 +++++++++-----
 llvm/unittests/ADT/APIntTest.cpp |  5 +++--
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/llvm/include/llvm/ADT/APInt.h b/llvm/include/llvm/ADT/APInt.h
index 65ba3f15305c78..a42dae8887392d 100644
--- a/llvm/include/llvm/ADT/APInt.h
+++ b/llvm/include/llvm/ADT/APInt.h
@@ -106,11 +106,26 @@ class [[nodiscard]] APInt {
   /// \param numBits the bit width of the constructed APInt
   /// \param val the initial value of the APInt
   /// \param isSigned how to treat signedness of val
-  APInt(unsigned numBits, uint64_t val, bool isSigned = false)
+  /// \param implicitTrunc allow implicit truncation of non-zero/sign bits of
+  ///                      val beyond the range of numBits
+  APInt(unsigned numBits, uint64_t val, bool isSigned = false,
+        bool implicitTrunc = true)
       : BitWidth(numBits) {
+    if (!implicitTrunc) {
+      if (BitWidth == 0) {
+        assert(val == 0 && "Value must be zero for 0-bit APInt");
+      } else if (isSigned) {
+        assert(llvm::isIntN(BitWidth, val) &&
+               "Value is not an N-bit signed value");
+      } else {
+        assert(llvm::isUIntN(BitWidth, val) &&
+               "Value is not an N-bit unsigned value");
+      }
+    }
     if (isSingleWord()) {
       U.VAL = val;
-      clearUnusedBits();
+      if (implicitTrunc || isSigned)
+        clearUnusedBits();
     } else {
       initSlowCase(val, isSigned);
     }
diff --git a/llvm/lib/Support/APInt.cpp b/llvm/lib/Support/APInt.cpp
index fe22e9ba04b6f5..78d573966c6c99 100644
--- a/llvm/lib/Support/APInt.cpp
+++ b/llvm/lib/Support/APInt.cpp
@@ -234,7 +234,8 @@ APInt& APInt::operator-=(uint64_t RHS) {
 APInt APInt::operator*(const APInt& RHS) const {
   assert(BitWidth == RHS.BitWidth && "Bit widths must be the same");
   if (isSingleWord())
-    return APInt(BitWidth, U.VAL * RHS.U.VAL);
+    return APInt(BitWidth, U.VAL * RHS.U.VAL, /*isSigned=*/false,
+                 /*implicitTrunc=*/true);
 
   APInt Result(getMemory(getNumWords()), getBitWidth());
   tcMultiply(Result.U.pVal, U.pVal, RHS.U.pVal, getNumWords());
@@ -455,7 +456,8 @@ APInt APInt::extractBits(unsigned numBits, unsigned bitPosition) const {
          "Illegal bit extraction");
 
   if (isSingleWord())
-    return APInt(numBits, U.VAL >> bitPosition);
+    return APInt(numBits, U.VAL >> bitPosition, /*isSigned=*/false,
+                 /*implicitTrunc=*/true);
 
   unsigned loBit = whichBit(bitPosition);
   unsigned loWord = whichWord(bitPosition);
@@ -463,7 +465,8 @@ APInt APInt::extractBits(unsigned numBits, unsigned bitPosition) const {
 
   // Single word result extracting bits from a single word source.
   if (loWord == hiWord)
-    return APInt(numBits, U.pVal[loWord] >> loBit);
+    return APInt(numBits, U.pVal[loWord] >> loBit, /*isSigned=*/false,
+                 /*implicitTrunc=*/true);
 
   // Extracting bits that start on a source word boundary can be done
   // as a fast memory copy.
@@ -907,7 +910,8 @@ APInt APInt::trunc(unsigned width) const {
   assert(width <= BitWidth && "Invalid APInt Truncate request");
 
   if (width <= APINT_BITS_PER_WORD)
-    return APInt(width, getRawData()[0]);
+    return APInt(width, getRawData()[0], /*isSigned=*/false,
+                 /*implicitTrunc=*/true);
 
   if (width == BitWidth)
     return *this;
@@ -955,7 +959,7 @@ APInt APInt::sext(unsigned Width) const {
   assert(Width >= BitWidth && "Invalid APInt SignExtend request");
 
   if (Width <= APINT_BITS_PER_WORD)
-    return APInt(Width, SignExtend64(U.VAL, BitWidth));
+    return APInt(Width, SignExtend64(U.VAL, BitWidth), /*isSigned=*/true);
 
   if (Width == BitWidth)
     return *this;
diff --git a/llvm/unittests/ADT/APIntTest.cpp b/llvm/unittests/ADT/APIntTest.cpp
index eb4b847185f53b..fff29d24a05299 100644
--- a/llvm/unittests/ADT/APIntTest.cpp
+++ b/llvm/unittests/ADT/APIntTest.cpp
@@ -220,11 +220,12 @@ TEST(APIntTest, i256) {
 }
 
 TEST(APIntTest, i1) {
-  const APInt neg_two(1, static_cast<uint64_t>(-2), true);
+  const APInt neg_two(1, static_cast<uint64_t>(-2), true,
+                      /*implicitTrunc=*/true);
   const APInt neg_one(1, static_cast<uint64_t>(-1), true);
   const APInt zero(1, 0);
   const APInt one(1, 1);
-  const APInt two(1, 2);
+  const APInt two(1, 2, false, /*implicitTrunc=*/true);
 
   EXPECT_EQ(0, neg_two.getSExtValue());
   EXPECT_EQ(-1, neg_one.getSExtValue());