[libc-commits] [libc] [libc] Make `BigInt` bitwise shift consistent with regular integral semantics. (PR #87874)
Guillaume Chatelet via libc-commits
libc-commits at lists.llvm.org
Sat Apr 6 10:24:43 PDT 2024
https://github.com/gchatelet updated https://github.com/llvm/llvm-project/pull/87874
>From 22c269c8370981468f0364e7a2932a914b6400a5 Mon Sep 17 00:00:00 2001
From: Guillaume Chatelet <gchatelet at google.com>
Date: Fri, 5 Apr 2024 09:49:07 +0000
Subject: [PATCH 1/2] [libc] Make `BigInt` bitwise shift consistent with
regular integral semantics.
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 https://github.com/llvm/llvm-project/pull/86137#issuecomment-2034133868.
---
libc/src/__support/FPUtil/dyadic_float.h | 6 ++++--
libc/src/__support/big_int.h | 10 ++++------
libc/test/src/__support/big_int_test.cpp | 16 ++++++----------
3 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/libc/src/__support/FPUtil/dyadic_float.h b/libc/src/__support/FPUtil/dyadic_float.h
index 49fb11971104b3..12a69228d36c7b 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/big_int.h b/libc/src/__support/big_int.h
index c1e55ceef21113..f722a81d357d4f 100644
--- a/libc/src/__support/big_int.h
+++ b/libc/src/__support/big_int.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/big_int_test.cpp b/libc/test/src/__support/big_int_test.cpp
index fadec0cc313b16..db12981a4a670f 100644
--- a/libc/test/src/__support/big_int_test.cpp
+++ b/libc/test/src/__support/big_int_test.cpp
@@ -193,8 +193,12 @@ 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) {
+ // Prevent UB when i == T::BITS.
+ 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 +563,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 +589,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);
>From 39c02566c8e15d57b16e07fcc0d8a4d4ac7b376b Mon Sep 17 00:00:00 2001
From: Guillaume Chatelet <gchatelet at google.com>
Date: Sat, 6 Apr 2024 09:25:07 +0000
Subject: [PATCH 2/2] Fix test that was still shifting by the wrong amount
---
libc/test/src/__support/big_int_test.cpp | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/libc/test/src/__support/big_int_test.cpp b/libc/test/src/__support/big_int_test.cpp
index db12981a4a670f..1c4f0ac29171fa 100644
--- a/libc/test/src/__support/big_int_test.cpp
+++ b/libc/test/src/__support/big_int_test.cpp
@@ -192,13 +192,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 zero_or = [i](T value) {
- // Prevent UB when i == T::BITS.
- 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
+ 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 int zeros = i;
const int ones = T::BITS - zeros;
ASSERT_EQ(cpp::countr_one(r_one), ones);
More information about the libc-commits
mailing list