[libc-commits] [libc] [llvm] [libc] Fix forward missing `BigInt` specialization of `mask_leading_ones` / `mask_trailing_ones` (PR #84325)

Guillaume Chatelet via libc-commits libc-commits at lists.llvm.org
Thu Mar 7 10:56:59 PST 2024


https://github.com/gchatelet updated https://github.com/llvm/llvm-project/pull/84325

>From 90b50ce1d86ac61d51c0f1358b4c0c55bb0c898a Mon Sep 17 00:00:00 2001
From: Guillaume Chatelet <gchatelet at google.com>
Date: Thu, 7 Mar 2024 14:24:43 +0000
Subject: [PATCH 1/4] [libc] Fix forward missing `BigInt` specialization of
 `mask_leading_ones` / `mask_trailing_ones`

#84299 broke the arm32 build, this patch fixes it forward.
---
 libc/src/__support/UInt.h                     | 46 +++++++++++++++
 libc/src/__support/math_extras.h              | 13 ++---
 libc/test/src/__support/CMakeLists.txt        |  2 +
 libc/test/src/__support/math_extras_test.cpp  | 57 ++++++++++++-------
 .../libc/test/src/__support/BUILD.bazel       |  6 +-
 5 files changed, 93 insertions(+), 31 deletions(-)

diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index b3d8f00b9a01a5..44291218fb7bc0 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -1054,6 +1054,52 @@ rotr(T value, int rotate) {
   return (value >> rotate) | (value << (N - rotate));
 }
 
+// Specialization of mask_trailing_ones ('math_extras.h') for BigInt.
+template <typename T, size_t count>
+LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, T>
+mask_trailing_ones() {
+  static_assert(!T::SIGNED);
+  if (count == 0)
+    return T();
+  constexpr unsigned T_BITS = CHAR_BIT * sizeof(T);
+  static_assert(count <= T_BITS && "Invalid bit index");
+  T out;
+  size_t lo_index = 0;
+  for (auto &word : out.val) {
+    if (count < lo_index)
+      word = 0;
+    else if (count > lo_index + T::WORD_SIZE)
+      word = -1;
+    else
+      word = mask_trailing_ones<T::word_type, count % T::WORD_SIZE>();
+    lo_index += T::WORD_SIZE;
+  }
+  return out;
+}
+
+// Specialization of mask_leading_ones ('math_extras.h') for BigInt.
+template <typename T, size_t count>
+LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, T>
+mask_leading_ones() {
+  static_assert(!T::SIGNED);
+  if (count == 0)
+    return T();
+  constexpr unsigned T_BITS = CHAR_BIT * sizeof(T);
+  static_assert(count <= T_BITS && "Invalid bit index");
+  T out;
+  size_t lo_index = 0;
+  for (auto &word : out.val) {
+    if (count < lo_index)
+      word = -1;
+    else if (count > lo_index + T::WORD_SIZE)
+      word = 0;
+    else
+      word = mask_leading_ones<T::word_type, count % T::WORD_SIZE>();
+    lo_index += T::WORD_SIZE;
+  }
+  return out;
+}
+
 } // namespace LIBC_NAMESPACE::cpp
 
 #endif // LLVM_LIBC_SRC___SUPPORT_UINT_H
diff --git a/libc/src/__support/math_extras.h b/libc/src/__support/math_extras.h
index 7a89fbb11b2a9e..c6b458ddecdabf 100644
--- a/libc/src/__support/math_extras.h
+++ b/libc/src/__support/math_extras.h
@@ -20,21 +20,18 @@ namespace LIBC_NAMESPACE {
 // Create a bitmask with the count right-most bits set to 1, and all other bits
 // set to 0.  Only unsigned types are allowed.
 template <typename T, size_t count>
-LIBC_INLINE constexpr T mask_trailing_ones() {
-  static_assert(cpp::is_unsigned_v<T>);
+LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, T>
+mask_trailing_ones() {
   constexpr unsigned T_BITS = CHAR_BIT * sizeof(T);
   static_assert(count <= T_BITS && "Invalid bit index");
-  // It's important not to initialize T with -1, since T may be BigInt which
-  // will take -1 as a uint64_t and only initialize the low 64 bits.
-  constexpr T ALL_ZEROES(0);
-  constexpr T ALL_ONES(~ALL_ZEROES); // bitwise NOT performs integer promotion.
-  return count == 0 ? 0 : (ALL_ONES >> (T_BITS - count));
+  return count == 0 ? 0 : (T(-1) >> (T_BITS - count));
 }
 
 // Create a bitmask with the count left-most bits set to 1, and all other bits
 // set to 0.  Only unsigned types are allowed.
 template <typename T, size_t count>
-LIBC_INLINE constexpr T mask_leading_ones() {
+LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, T>
+mask_leading_ones() {
   constexpr T MASK(mask_trailing_ones<T, CHAR_BIT * sizeof(T) - count>());
   return T(~MASK); // bitwise NOT performs integer promotion.
 }
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index 8c861b576f9b1b..adbacb9728ccd4 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -27,7 +27,9 @@ add_libc_test(
   SRCS
     math_extras_test.cpp
   DEPENDS
+    libc.src.__support.integer_literals
     libc.src.__support.math_extras
+    libc.src.__support.uint128
 )
 
 add_libc_test(
diff --git a/libc/test/src/__support/math_extras_test.cpp b/libc/test/src/__support/math_extras_test.cpp
index e55d995592cc1c..7e293268729ec7 100644
--- a/libc/test/src/__support/math_extras_test.cpp
+++ b/libc/test/src/__support/math_extras_test.cpp
@@ -6,34 +6,47 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "src/__support/UInt128.h" // UInt128
+#include "src/__support/integer_literals.h"
 #include "src/__support/math_extras.h"
 #include "test/UnitTest/Test.h"
 
 namespace LIBC_NAMESPACE {
 
 TEST(LlvmLibcBlockMathExtrasTest, mask_trailing_ones) {
-  EXPECT_EQ(uint8_t(0), (mask_leading_ones<uint8_t, 0>()));
-  EXPECT_EQ(uint8_t(0), (mask_trailing_ones<uint8_t, 0>()));
-  EXPECT_EQ(uint16_t(0), (mask_leading_ones<uint16_t, 0>()));
-  EXPECT_EQ(uint16_t(0), (mask_trailing_ones<uint16_t, 0>()));
-  EXPECT_EQ(uint32_t(0), (mask_leading_ones<uint32_t, 0>()));
-  EXPECT_EQ(uint32_t(0), (mask_trailing_ones<uint32_t, 0>()));
-  EXPECT_EQ(uint64_t(0), (mask_leading_ones<uint64_t, 0>()));
-  EXPECT_EQ(uint64_t(0), (mask_trailing_ones<uint64_t, 0>()));
-
-  EXPECT_EQ(uint32_t(0x00000003), (mask_trailing_ones<uint32_t, 2>()));
-  EXPECT_EQ(uint32_t(0xC0000000), (mask_leading_ones<uint32_t, 2>()));
-
-  EXPECT_EQ(uint32_t(0x000007FF), (mask_trailing_ones<uint32_t, 11>()));
-  EXPECT_EQ(uint32_t(0xFFE00000), (mask_leading_ones<uint32_t, 11>()));
-
-  EXPECT_EQ(uint32_t(0xFFFFFFFF), (mask_trailing_ones<uint32_t, 32>()));
-  EXPECT_EQ(uint32_t(0xFFFFFFFF), (mask_leading_ones<uint32_t, 32>()));
-  EXPECT_EQ(uint64_t(0xFFFFFFFFFFFFFFFF), (mask_trailing_ones<uint64_t, 64>()));
-  EXPECT_EQ(uint64_t(0xFFFFFFFFFFFFFFFF), (mask_leading_ones<uint64_t, 64>()));
-
-  EXPECT_EQ(uint64_t(0x0000FFFFFFFFFFFF), (mask_trailing_ones<uint64_t, 48>()));
-  EXPECT_EQ(uint64_t(0xFFFFFFFFFFFF0000), (mask_leading_ones<uint64_t, 48>()));
+  EXPECT_EQ(0_u8, (mask_leading_ones<uint8_t, 0>()));
+  EXPECT_EQ(0_u8, (mask_trailing_ones<uint8_t, 0>()));
+  EXPECT_EQ(0_u16, (mask_leading_ones<uint16_t, 0>()));
+  EXPECT_EQ(0_u16, (mask_trailing_ones<uint16_t, 0>()));
+  EXPECT_EQ(0_u32, (mask_leading_ones<uint32_t, 0>()));
+  EXPECT_EQ(0_u32, (mask_trailing_ones<uint32_t, 0>()));
+  EXPECT_EQ(0_u64, (mask_leading_ones<uint64_t, 0>()));
+  EXPECT_EQ(0_u64, (mask_trailing_ones<uint64_t, 0>()));
+
+  EXPECT_EQ(0x00000003_u32, (mask_trailing_ones<uint32_t, 2>()));
+  EXPECT_EQ(0xC0000000_u32, (mask_leading_ones<uint32_t, 2>()));
+
+  EXPECT_EQ(0x000007FF_u32, (mask_trailing_ones<uint32_t, 11>()));
+  EXPECT_EQ(0xFFE00000_u32, (mask_leading_ones<uint32_t, 11>()));
+
+  EXPECT_EQ(0xFFFFFFFF_u32, (mask_trailing_ones<uint32_t, 32>()));
+  EXPECT_EQ(0xFFFFFFFF_u32, (mask_leading_ones<uint32_t, 32>()));
+  EXPECT_EQ(0xFFFFFFFFFFFFFFFF_u64, (mask_trailing_ones<uint64_t, 64>()));
+  EXPECT_EQ(0xFFFFFFFFFFFFFFFF_u64, (mask_leading_ones<uint64_t, 64>()));
+
+  EXPECT_EQ(0x0000FFFFFFFFFFFF_u64, (mask_trailing_ones<uint64_t, 48>()));
+  EXPECT_EQ(0xFFFFFFFFFFFF0000_u64, (mask_leading_ones<uint64_t, 48>()));
+
+  EXPECT_EQ(0_u128, (mask_trailing_ones<UInt128, 0>()));
+  EXPECT_EQ(0_u128, (mask_leading_ones<UInt128, 0>()));
+  EXPECT_EQ(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF_u128,
+            (mask_trailing_ones<UInt128, 128>()));
+  EXPECT_EQ(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF_u128,
+            (mask_leading_ones<UInt128, 128>()));
+  EXPECT_EQ(0x0000FFFFFFFFFFFFFFFFFFFFFFFFFFFF_u128,
+            (mask_trailing_ones<UInt128, 112>()));
+  EXPECT_EQ(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFF0000_u128,
+            (mask_leading_ones<UInt128, 112>()));
 }
 
 } // namespace LIBC_NAMESPACE
diff --git a/utils/bazel/llvm-project-overlay/libc/test/src/__support/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/test/src/__support/BUILD.bazel
index 8e94a84f586f4c..19d4c7869799a0 100644
--- a/utils/bazel/llvm-project-overlay/libc/test/src/__support/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/libc/test/src/__support/BUILD.bazel
@@ -13,7 +13,11 @@ licenses(["notice"])
 libc_test(
     name = "math_extras_test",
     srcs = ["math_extras_test.cpp"],
-    deps = ["//libc:__support_math_extras"],
+    deps = [
+        "//libc:__support_integer_literals",
+        "//libc:__support_math_extras",
+        "//libc:__support_uint128",
+    ],
 )
 
 # This test is currently disabled because of an issue in

>From eb9397269391f142d5e81b4c37baed14b9a5d83d Mon Sep 17 00:00:00 2001
From: Guillaume Chatelet <gchatelet at google.com>
Date: Thu, 7 Mar 2024 16:08:36 +0000
Subject: [PATCH 2/4] Fix implementation of mask_(trailing|leading)_ones for
 BigInt

---
 libc/src/__support/UInt.h                    | 35 ++++++++++++--------
 libc/test/src/__support/math_extras_test.cpp | 20 ++++++++---
 2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index 44291218fb7bc0..f3676b9980bd68 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -1054,6 +1054,10 @@ rotr(T value, int rotate) {
   return (value >> rotate) | (value << (N - rotate));
 }
 
+} // namespace LIBC_NAMESPACE::cpp
+
+namespace LIBC_NAMESPACE {
+
 // Specialization of mask_trailing_ones ('math_extras.h') for BigInt.
 template <typename T, size_t count>
 LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, T>
@@ -1063,16 +1067,18 @@ mask_trailing_ones() {
     return T();
   constexpr unsigned T_BITS = CHAR_BIT * sizeof(T);
   static_assert(count <= T_BITS && "Invalid bit index");
+  using word_type = typename T::word_type;
   T out;
-  size_t lo_index = 0;
+  const int chunk_index_containing_bit = static_cast<int>(count / T::WORD_SIZE);
+  int index = 0;
   for (auto &word : out.val) {
-    if (count < lo_index)
-      word = 0;
-    else if (count > lo_index + T::WORD_SIZE)
+    if (index < chunk_index_containing_bit)
       word = -1;
+    else if (index > chunk_index_containing_bit)
+      word = 0;
     else
-      word = mask_trailing_ones<T::word_type, count % T::WORD_SIZE>();
-    lo_index += T::WORD_SIZE;
+      word = mask_trailing_ones<word_type, count % T::WORD_SIZE>();
+    ++index;
   }
   return out;
 }
@@ -1086,20 +1092,23 @@ mask_leading_ones() {
     return T();
   constexpr unsigned T_BITS = CHAR_BIT * sizeof(T);
   static_assert(count <= T_BITS && "Invalid bit index");
+  using word_type = typename T::word_type;
   T out;
-  size_t lo_index = 0;
+  const int chunk_index_containing_bit =
+      static_cast<int>((T::BITS - count - 1ULL) / T::WORD_SIZE);
+  int index = 0;
   for (auto &word : out.val) {
-    if (count < lo_index)
-      word = -1;
-    else if (count > lo_index + T::WORD_SIZE)
+    if (index < chunk_index_containing_bit)
       word = 0;
+    else if (index > chunk_index_containing_bit)
+      word = -1;
     else
-      word = mask_leading_ones<T::word_type, count % T::WORD_SIZE>();
-    lo_index += T::WORD_SIZE;
+      word = mask_leading_ones<word_type, count % T::WORD_SIZE>();
+    ++index;
   }
   return out;
 }
 
-} // namespace LIBC_NAMESPACE::cpp
+} // namespace LIBC_NAMESPACE
 
 #endif // LLVM_LIBC_SRC___SUPPORT_UINT_H
diff --git a/libc/test/src/__support/math_extras_test.cpp b/libc/test/src/__support/math_extras_test.cpp
index 7e293268729ec7..ed064363d446bb 100644
--- a/libc/test/src/__support/math_extras_test.cpp
+++ b/libc/test/src/__support/math_extras_test.cpp
@@ -39,14 +39,26 @@ TEST(LlvmLibcBlockMathExtrasTest, mask_trailing_ones) {
 
   EXPECT_EQ(0_u128, (mask_trailing_ones<UInt128, 0>()));
   EXPECT_EQ(0_u128, (mask_leading_ones<UInt128, 0>()));
+
+  EXPECT_EQ(0x00000000000000007FFFFFFFFFFFFFFF_u128,
+            (mask_trailing_ones<UInt128, 63>()));
+  EXPECT_EQ(0xFFFFFFFFFFFFFFFE0000000000000000_u128,
+            (mask_leading_ones<UInt128, 63>()));
+
+  EXPECT_EQ(0x0000000000000000FFFFFFFFFFFFFFFF_u128,
+            (mask_trailing_ones<UInt128, 64>()));
+  EXPECT_EQ(0xFFFFFFFFFFFFFFFF0000000000000000_u128,
+            (mask_leading_ones<UInt128, 64>()));
+
+  EXPECT_EQ(0x0000000000000001FFFFFFFFFFFFFFFF_u128,
+            (mask_trailing_ones<UInt128, 65>()));
+  EXPECT_EQ(0xFFFFFFFFFFFFFFFF8000000000000000_u128,
+            (mask_leading_ones<UInt128, 65>()));
+
   EXPECT_EQ(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF_u128,
             (mask_trailing_ones<UInt128, 128>()));
   EXPECT_EQ(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF_u128,
             (mask_leading_ones<UInt128, 128>()));
-  EXPECT_EQ(0x0000FFFFFFFFFFFFFFFFFFFFFFFFFFFF_u128,
-            (mask_trailing_ones<UInt128, 112>()));
-  EXPECT_EQ(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFF0000_u128,
-            (mask_leading_ones<UInt128, 112>()));
 }
 
 } // namespace LIBC_NAMESPACE

>From 48a876b9d07bd066c3d42c6f75bfc64495bbf7c1 Mon Sep 17 00:00:00 2001
From: Guillaume Chatelet <gchatelet at google.com>
Date: Thu, 7 Mar 2024 16:09:26 +0000
Subject: [PATCH 3/4] Fix other places where functions whould work with
 is_integral_v and is_big_int_v

---
 libc/src/stdio/printf_core/float_dec_converter.h | 7 +++++--
 libc/test/UnitTest/CMakeLists.txt                | 1 +
 libc/test/UnitTest/StringUtils.h                 | 3 ++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/libc/src/stdio/printf_core/float_dec_converter.h b/libc/src/stdio/printf_core/float_dec_converter.h
index a6c68329e66023..27d229a3e42cb5 100644
--- a/libc/src/stdio/printf_core/float_dec_converter.h
+++ b/libc/src/stdio/printf_core/float_dec_converter.h
@@ -12,6 +12,7 @@
 #include "src/__support/CPP/string_view.h"
 #include "src/__support/FPUtil/FPBits.h"
 #include "src/__support/FPUtil/rounding_mode.h"
+#include "src/__support/UInt.h" // cpp::is_big_int_v
 #include "src/__support/float_to_string.h"
 #include "src/__support/integer_to_string.h"
 #include "src/__support/libc_assert.h"
@@ -33,7 +34,8 @@ using ExponentString =
 
 // Returns true if value is divisible by 2^p.
 template <typename T>
-LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_integral_v<T>, bool>
+LIBC_INLINE constexpr cpp::enable_if_t<
+    cpp::is_integral_v<T> || cpp::is_big_int_v<T>, bool>
 multiple_of_power_of_2(T value, uint32_t p) {
   return (value & ((T(1) << p) - 1)) == 0;
 }
@@ -76,7 +78,8 @@ LIBC_INLINE RoundDirection get_round_direction(int last_digit, bool truncated,
 }
 
 template <typename T>
-LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_integral_v<T>, bool>
+LIBC_INLINE constexpr cpp::enable_if_t<
+    cpp::is_integral_v<T> || cpp::is_big_int_v<T>, bool>
 zero_after_digits(int32_t base_2_exp, int32_t digits_after_point, T mantissa,
                   const int32_t mant_width) {
   const int32_t required_twos = -base_2_exp - digits_after_point - 1;
diff --git a/libc/test/UnitTest/CMakeLists.txt b/libc/test/UnitTest/CMakeLists.txt
index 36837c553efce1..8a35f1204eb511 100644
--- a/libc/test/UnitTest/CMakeLists.txt
+++ b/libc/test/UnitTest/CMakeLists.txt
@@ -104,6 +104,7 @@ add_header_library(
   DEPENDS
     libc.src.__support.CPP.string
     libc.src.__support.CPP.type_traits
+    libc.src.__support.uint
 )
 
 add_unittest_framework_library(
diff --git a/libc/test/UnitTest/StringUtils.h b/libc/test/UnitTest/StringUtils.h
index 54cff97ceafb4e..1e3ba5715d23d6 100644
--- a/libc/test/UnitTest/StringUtils.h
+++ b/libc/test/UnitTest/StringUtils.h
@@ -11,12 +11,13 @@
 
 #include "src/__support/CPP/string.h"
 #include "src/__support/CPP/type_traits.h"
+#include "src/__support/UInt.h"
 
 namespace LIBC_NAMESPACE {
 
 // Return the first N hex digits of an integer as a string in upper case.
 template <typename T>
-cpp::enable_if_t<cpp::is_integral_v<T>, cpp::string>
+cpp::enable_if_t<cpp::is_integral_v<T> || cpp::is_big_int_v<T>, cpp::string>
 int_to_hex(T value, size_t length = sizeof(T) * 2) {
   cpp::string s(length, '0');
 

>From 3748da03b7c571ec788829816cec141b8b311950 Mon Sep 17 00:00:00 2001
From: Guillaume Chatelet <gchatelet at google.com>
Date: Thu, 7 Mar 2024 18:56:41 +0000
Subject: [PATCH 4/4] make chunk_index_containing_bit constexpr

---
 libc/src/__support/UInt.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index f3676b9980bd68..925de8764715da 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -1069,12 +1069,13 @@ mask_trailing_ones() {
   static_assert(count <= T_BITS && "Invalid bit index");
   using word_type = typename T::word_type;
   T out;
-  const int chunk_index_containing_bit = static_cast<int>(count / T::WORD_SIZE);
+  constexpr int CHUNK_INDEX_CONTAINING_BIT =
+      static_cast<int>(count / T::WORD_SIZE);
   int index = 0;
   for (auto &word : out.val) {
-    if (index < chunk_index_containing_bit)
+    if (index < CHUNK_INDEX_CONTAINING_BIT)
       word = -1;
-    else if (index > chunk_index_containing_bit)
+    else if (index > CHUNK_INDEX_CONTAINING_BIT)
       word = 0;
     else
       word = mask_trailing_ones<word_type, count % T::WORD_SIZE>();
@@ -1094,13 +1095,13 @@ mask_leading_ones() {
   static_assert(count <= T_BITS && "Invalid bit index");
   using word_type = typename T::word_type;
   T out;
-  const int chunk_index_containing_bit =
+  constexpr int CHUNK_INDEX_CONTAINING_BIT =
       static_cast<int>((T::BITS - count - 1ULL) / T::WORD_SIZE);
   int index = 0;
   for (auto &word : out.val) {
-    if (index < chunk_index_containing_bit)
+    if (index < CHUNK_INDEX_CONTAINING_BIT)
       word = 0;
-    else if (index > chunk_index_containing_bit)
+    else if (index > CHUNK_INDEX_CONTAINING_BIT)
       word = -1;
     else
       word = mask_leading_ones<word_type, count % T::WORD_SIZE>();



More information about the libc-commits mailing list