[libc] Make BigInt bitwise shift consistent with regular integral semantics.

[gchatelet]

This patch removes the test for cases where the shift operand is greater or equal to the bit width of the number. This is done for two reasons, first it makes BigInt consistent with regular integral bitwise shift semantics, and second it makes the shift operation faster. The shift operation is on the critical path for exp and log operations, see #86137 (comment).

[llvmbot]

@llvm/pr-subscribers-libc

Author: Guillaume Chatelet (gchatelet)

Changes

This patch removes the test for cases where the shift operand is greater or equal to the bit width of the number. This is done for two reasons, first it makes BigInt consistent with regular integral bitwise shift semantics, and second it makes the shift operation faster. The shift operation is on the critical path for exp and log operations, see #86137 (comment).

---

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

3 Files Affected:

• (modified) libc/src/__support/FPUtil/dyadic_float.h (+4-2)
• (modified) libc/src/__support/UInt.h (+4-6)
• (modified) libc/test/src/__support/uint_test.cpp (+3-10)

diff --git a/libc/src/__support/FPUtil/dyadic_float.h b/libc/src/__support/FPUtil/dyadic_float.h
index e0c205f52383ba..e4bc6421a4113c 100644
--- a/libc/src/__support/FPUtil/dyadic_float.h
+++ b/libc/src/__support/FPUtil/dyadic_float.h
@@ -122,7 +122,8 @@ template <size_t Bits> struct DyadicFloat {
 
     int exp_lo = exp_hi - static_cast<int>(PRECISION) - 1;
 
-    MantissaType m_hi(mantissa >> shift);
+    MantissaType m_hi =
+        shift >= MantissaType::BITS ? MantissaType(0) : mantissa >> shift;
 
     T d_hi = FPBits<T>::create_value(
                  sign, exp_hi,
@@ -130,7 +131,8 @@ template <size_t Bits> struct DyadicFloat {
                      IMPLICIT_MASK)
                  .get_val();
 
-    MantissaType round_mask = MantissaType(1) << (shift - 1);
+    MantissaType round_mask =
+        shift > MantissaType::BITS ? 0 : MantissaType(1) << (shift - 1);
     MantissaType sticky_mask = round_mask - MantissaType(1);
 
     bool round_bit = !(mantissa & round_mask).is_zero();
diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index c1e55ceef21113..f722a81d357d4f 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -249,18 +249,14 @@ LIBC_INLINE constexpr bool is_negative(cpp::array<word, N> &array) {
 enum Direction { LEFT, RIGHT };
 
 // A bitwise shift on an array of elements.
-// TODO: Make the result UB when 'offset' is greater or equal to the number of
-// bits in 'array'. This will allow for better code performance.
+// 'offset' must be less than TOTAL_BITS (i.e., sizeof(word) * CHAR_BIT * N)
+// otherwise the behavior is undefined.
 template <Direction direction, bool is_signed, typename word, size_t N>
 LIBC_INLINE constexpr cpp::array<word, N> shift(cpp::array<word, N> array,
                                                 size_t offset) {
   static_assert(direction == LEFT || direction == RIGHT);
   constexpr size_t WORD_BITS = cpp::numeric_limits<word>::digits;
   constexpr size_t TOTAL_BITS = N * WORD_BITS;
-  if (LIBC_UNLIKELY(offset == 0))
-    return array;
-  if (LIBC_UNLIKELY(offset >= TOTAL_BITS))
-    return {};
 #ifdef LIBC_TYPES_HAS_INT128
   if constexpr (TOTAL_BITS == 128) {
     using type = cpp::conditional_t<is_signed, __int128_t, __uint128_t>;
@@ -272,6 +268,8 @@ LIBC_INLINE constexpr cpp::array<word, N> shift(cpp::array<word, N> array,
     return cpp::bit_cast<cpp::array<word, N>>(tmp);
   }
 #endif
+  if (LIBC_UNLIKELY(offset == 0))
+    return array;
   const bool is_neg = is_signed && is_negative(array);
   constexpr auto at = [](size_t index) -> int {
     // reverse iteration when direction == LEFT.
diff --git a/libc/test/src/__support/uint_test.cpp b/libc/test/src/__support/uint_test.cpp
index 5696e54c73f363..d0c2f33ca768a7 100644
--- a/libc/test/src/__support/uint_test.cpp
+++ b/libc/test/src/__support/uint_test.cpp
@@ -193,8 +193,9 @@ TYPED_TEST(LlvmLibcUIntClassTest, Masks, Types) {
 TYPED_TEST(LlvmLibcUIntClassTest, CountBits, Types) {
   if constexpr (!T::SIGNED) {
     for (size_t i = 0; i <= T::BITS; ++i) {
-      const auto l_one = T::all_ones() << i; // 0b111...000
-      const auto r_one = T::all_ones() >> i; // 0b000...111
+      const auto zero_or = [i](T value) { return i == T::BITS ? 0 : value; };
+      const auto l_one = zero_or(T::all_ones() << i); // 0b111...000
+      const auto r_one = zero_or(T::all_ones() >> i); // 0b000...111
       const int zeros = i;
       const int ones = T::BITS - zeros;
       ASSERT_EQ(cpp::countr_one(r_one), ones);
@@ -559,10 +560,6 @@ TEST(LlvmLibcUIntClassTest, ShiftLeftTests) {
   LL_UInt128 result5({0, 0x2468ace000000000});
   EXPECT_EQ((val2 << 100), result5);
 
-  LL_UInt128 result6({0, 0});
-  EXPECT_EQ((val2 << 128), result6);
-  EXPECT_EQ((val2 << 256), result6);
-
   LL_UInt192 val3({1, 0, 0});
   LL_UInt192 result7({0, 1, 0});
   EXPECT_EQ((val3 << 64), result7);
@@ -589,10 +586,6 @@ TEST(LlvmLibcUIntClassTest, ShiftRightTests) {
   LL_UInt128 result5({0x0000000001234567, 0});
   EXPECT_EQ((val2 >> 100), result5);
 
-  LL_UInt128 result6({0, 0});
-  EXPECT_EQ((val2 >> 128), result6);
-  EXPECT_EQ((val2 >> 256), result6);
-
   LL_UInt128 v1({0x1111222233334444, 0xaaaabbbbccccdddd});
   LL_UInt128 r1({0xaaaabbbbccccdddd, 0});
   EXPECT_EQ((v1 >> 64), r1);

[lntue]

Do we need the checks here and below? The above computations guaranteed that 0 < shift < MantissaType::Bits = Bits.

[gchatelet]

Without these changes the tests are failing, so apparently shift may be greater or equal to BITS. I also ran the tests with -fsanitize=undefined using __uint128_t as the MantissaType in a separate patch and it reported an out of bound shift at these locations.

[lntue]

Are you able to extract example inputs / tests that fail without this?

[gchatelet]

The failing tests:

@llvm-project//libc/test/src/__support/FPUtil:dyadic_float_test          FAILED in 0.1s
@llvm-project//libc/test/src/math:ldexp_test                             FAILED in 0.0s
@llvm-project//libc/test/src/math:ldexpf_test                            FAILED in 0.0s
@llvm-project//libc/test/src/math:ldexpl_test                            FAILED in 0.0s
@llvm-project//libc/test/src/math:scalbn_test                            FAILED in 0.1s
@llvm-project//libc/test/src/math:scalbnf_test                           FAILED in 0.1s
@llvm-project//libc/test/src/math:scalbnl_test                           FAILED in 0.0s
@llvm-project//libc/test/src/math/smoke:ldexpf128_test                   FAILED in 0.0s

A few failure logs below

[ RUN      ] LlvmLibcScalbnTest.UnderflowToZeroOnNormal
external/llvm-project/libc/test/src/math/LdExpTest.h:79: FAILURE
Failed to match x > 0 ? zero : neg_zero against LIBC_NAMESPACE::testing::getMatcher< LIBC_NAMESPACE::testing::TestCond::EQ>(func(x, -exp)).
Expected floating point value: 0x000091A0 = (S: 0, E: 0x0000, M: 0x000091A0)
Actual floating point value: 0x00000000 = (S: 0, E: 0x0000, M: 0x00000000)
[  FAILED  ] LlvmLibcScalbnTest.UnderflowToZeroOnNormal
[ RUN      ] LlvmLibcScalbnTest.UnderflowToZeroOnSubnormal
external/llvm-project/libc/test/src/math/LdExpTest.h:92: FAILURE
Failed to match x > 0 ? zero : neg_zero against LIBC_NAMESPACE::testing::getMatcher< LIBC_NAMESPACE::testing::TestCond::EQ>(func(x, -exp)).
Expected floating point value: 0x00012340 = (S: 0, E: 0x0000, M: 0x00012340)
Actual floating point value: 0x00000000 = (S: 0, E: 0x0000, M: 0x00000000)
[  FAILED  ] LlvmLibcScalbnTest.UnderflowToZeroOnSubnormal
[ RUN      ] LlvmLibcScalbnTest.NormalOperation
external/llvm-project/libc/test/src/math/LdExpTest.h:118: FAILURE
Failed to match x / two_to_exp against LIBC_NAMESPACE::testing::getMatcher< LIBC_NAMESPACE::testing::TestCond::EQ>(func(x, -exp)).
Expected floating point value: 0x00123401 = (S: 0, E: 0x0000, M: 0x00123401)
Actual floating point value: 0x00000001 = (S: 0, E: 0x0000, M: 0x00000001)
[  FAILED  ] LlvmLibcScalbnTest.NormalOperation

---

[ RUN      ] LlvmLibcDyadicFloatTest.EdgeRangesFloat
external/llvm-project/libc/test/src/__support/FPUtil/dyadic_float_test.cpp:89: FAILURE
Failed to match static_cast<float>(z) against LIBC_NAMESPACE::testing::getMatcher< LIBC_NAMESPACE::testing::TestCond::EQ>(Bits::zero().get_val()).
Expected floating point value: 0x00000000 = (S: 0, E: 0x0000, M: 0x00000000)
Actual floating point value: 0xF4800000 = (S: 1, E: 0x00E9, M: 0x00000000)
[  FAILED  ] LlvmLibcDyadicFloatTest.EdgeRangesFloat

[lntue]

Thanks, I'll take a look.