[libc] Remove UB specializations of type traits for `BigInt` #84035

gchatelet · 2024-03-05T16:05:02Z

The standard specifies that it it UB to specialize the following traits:

std::is_integral
std::is_unsigned
std::make_unsigned
std::make_signed

This patch:

Removes specializations for BigInt
Transforms SFINAE for bit.h functions from template parameter to
return type (This makes specialization easier).
Adds BigInt specialization for bit.h functions.
Fixes code depending on previous specializations.

The standard specifies that it it UB to specialize the following traits: - std::is_integral - std::is_unsigned - std::make_unsigned - std::make_signed This patch: - Removes specializations for BigInt - Transforms SFINAE for bit.h functions from template parameter to return type (This makes specialization easier). - Adds BigInt specialization for bit.h functions. - Fixes code depending on previous specializations.

llvmbot · 2024-03-05T16:05:34Z

@llvm/pr-subscribers-libc

Author: Guillaume Chatelet (gchatelet)

Changes

The standard specifies that it it UB to specialize the following traits:

std::is_integral
std::is_unsigned
std::make_unsigned
std::make_signed

This patch:

Removes specializations for BigInt
Transforms SFINAE for bit.h functions from template parameter to
return type (This makes specialization easier).
Adds BigInt specialization for bit.h functions.
Fixes code depending on previous specializations.

Patch is 27.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84035.diff

11 Files Affected:

(modified) libc/src/__support/CMakeLists.txt (+1)
(modified) libc/src/__support/CPP/bit.h (+62-43)
(modified) libc/src/__support/UInt.h (+181-57)
(modified) libc/src/__support/float_to_string.h (+1-1)
(modified) libc/src/__support/integer_to_string.h (+16-3)
(modified) libc/test/UnitTest/CMakeLists.txt (+1)
(modified) libc/test/UnitTest/LibcTest.cpp (+5-5)
(modified) libc/test/UnitTest/LibcTest.h (+1)
(modified) libc/test/UnitTest/TestLogger.cpp (+5-3)
(modified) utils/bazel/llvm-project-overlay/libc/BUILD.bazel (+1)
(modified) utils/bazel/llvm-project-overlay/libc/test/UnitTest/BUILD.bazel (+1)

diff --git a/libc/src/__support/CMakeLists.txt b/libc/src/__support/CMakeLists.txt
index 1a4b3e9a2145c0..17c04aa57e6fd6 100644
--- a/libc/src/__support/CMakeLists.txt
+++ b/libc/src/__support/CMakeLists.txt
@@ -95,6 +95,7 @@ add_header_library(
   HDRS
     integer_to_string.h
   DEPENDS
+    .uint
     libc.src.__support.common
     libc.src.__support.CPP.algorithm
     libc.src.__support.CPP.limits
diff --git a/libc/src/__support/CPP/bit.h b/libc/src/__support/CPP/bit.h
index 7d11e7d5c497e0..496ca16b6d6017 100644
--- a/libc/src/__support/CPP/bit.h
+++ b/libc/src/__support/CPP/bit.h
@@ -27,13 +27,14 @@ namespace LIBC_NAMESPACE::cpp {
 
 // This implementation of bit_cast requires trivially-constructible To, to avoid
 // UB in the implementation.
-template <
-    typename To, typename From,
-    typename = cpp::enable_if_t<sizeof(To) == sizeof(From) &&
-                                cpp::is_trivially_constructible<To>::value &&
-                                cpp::is_trivially_copyable<To>::value &&
-                                cpp::is_trivially_copyable<From>::value>>
-LIBC_INLINE constexpr To bit_cast(const From &from) {
+template <typename To, typename From>
+LIBC_INLINE constexpr cpp::enable_if_t<
+    (sizeof(To) == sizeof(From)) &&
+        cpp::is_trivially_constructible<To>::value &&
+        cpp::is_trivially_copyable<To>::value &&
+        cpp::is_trivially_copyable<From>::value,
+    To>
+bit_cast(const From &from) {
   MSAN_UNPOISON(&from, sizeof(From));
 #if LIBC_HAS_BUILTIN(__builtin_bit_cast)
   return __builtin_bit_cast(To, from);
@@ -51,8 +52,10 @@ LIBC_INLINE constexpr To bit_cast(const From &from) {
 #endif // LIBC_HAS_BUILTIN(__builtin_bit_cast)
 }
 
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr bool has_single_bit(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>,
+                                                     bool>
+has_single_bit(T value) {
   return (value != 0) && ((value & (value - 1)) == 0);
 }
 
@@ -70,8 +73,9 @@ template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
 /// Only unsigned integral types are allowed.
 ///
 /// Returns cpp::numeric_limits<T>::digits on an input of 0.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int countr_zero(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+countr_zero(T value) {
   if (!value)
     return cpp::numeric_limits<T>::digits;
   if (value & 0x1)
@@ -103,8 +107,9 @@ ADD_SPECIALIZATION(countr_zero, unsigned long long, __builtin_ctzll)
 /// Only unsigned integral types are allowed.
 ///
 /// Returns cpp::numeric_limits<T>::digits on an input of 0.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int countl_zero(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+countl_zero(T value) {
   if (!value)
     return cpp::numeric_limits<T>::digits;
   // Bisection method.
@@ -135,8 +140,9 @@ ADD_SPECIALIZATION(countl_zero, unsigned long long, __builtin_clzll)
 /// Only unsigned integral types are allowed.
 ///
 /// Returns cpp::numeric_limits<T>::digits on an input of all ones.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int countl_one(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+countl_one(T value) {
   return cpp::countl_zero<T>(~value);
 }
 
@@ -147,8 +153,9 @@ template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
 /// Only unsigned integral types are allowed.
 ///
 /// Returns cpp::numeric_limits<T>::digits on an input of all ones.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int countr_one(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+countr_one(T value) {
   return cpp::countr_zero<T>(~value);
 }
 
@@ -156,8 +163,9 @@ template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
 /// Returns 0 otherwise.
 ///
 /// Ex. bit_width(5) == 3.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int bit_width(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+bit_width(T value) {
   return cpp::numeric_limits<T>::digits - cpp::countl_zero(value);
 }
 
@@ -165,8 +173,9 @@ template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
 /// nonzero.  Returns 0 otherwise.
 ///
 /// Ex. bit_floor(5) == 4.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr T bit_floor(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, T>
+bit_floor(T value) {
   if (!value)
     return 0;
   return T(1) << (cpp::bit_width(value) - 1);
@@ -179,8 +188,9 @@ template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
 ///
 /// The return value is undefined if the input is larger than the largest power
 /// of two representable in T.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr T bit_ceil(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, T>
+bit_ceil(T value) {
   if (value < 2)
     return 1;
   return T(1) << cpp::bit_width<T>(value - 1u);
@@ -190,28 +200,31 @@ template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
 // from https://blog.regehr.org/archives/1063.
 
 // Forward-declare rotr so that rotl can use it.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr T rotr(T value, int rotate);
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, T>
+rotr(T value, int rotate);
 
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr T rotl(T value, int rotate) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, T>
+rotl(T value, int rotate) {
   constexpr unsigned N = cpp::numeric_limits<T>::digits;
   rotate = rotate % N;
   if (!rotate)
     return value;
   if (rotate < 0)
-    return cpp::rotr(value, -rotate);
+    return cpp::rotr<T>(value, -rotate);
   return (value << rotate) | (value >> (N - rotate));
 }
 
-template <typename T, typename>
-[[nodiscard]] LIBC_INLINE constexpr T rotr(T value, int rotate) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, T>
+rotr(T value, int rotate) {
   constexpr unsigned N = cpp::numeric_limits<T>::digits;
   rotate = rotate % N;
   if (!rotate)
     return value;
   if (rotate < 0)
-    return cpp::rotl(value, -rotate);
+    return cpp::rotl<T>(value, -rotate);
   return (value >> rotate) | (value << (N - rotate));
 }
 
@@ -226,33 +239,38 @@ LIBC_INLINE constexpr To bit_or_static_cast(const From &from) {
   }
 }
 
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int first_leading_zero(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+first_leading_zero(T value) {
   return value == cpp::numeric_limits<T>::max() ? 0 : countl_one(value) + 1;
 }
 
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int first_leading_one(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+first_leading_one(T value) {
   return first_leading_zero(static_cast<T>(~value));
 }
 
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int first_trailing_zero(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+first_trailing_zero(T value) {
   return value == cpp::numeric_limits<T>::max()
              ? 0
              : countr_zero(static_cast<T>(~value)) + 1;
 }
 
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int first_trailing_one(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+first_trailing_one(T value) {
   return value == cpp::numeric_limits<T>::max() ? 0 : countr_zero(value) + 1;
 }
 
 /// Count number of 1's aka population count or hamming weight.
 ///
 /// Only unsigned integral types are allowed.
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int count_ones(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+count_ones(T value) {
   int count = 0;
   for (int i = 0; i != cpp::numeric_limits<T>::digits; ++i)
     if ((value >> i) & 0x1)
@@ -272,8 +290,9 @@ ADD_SPECIALIZATION(unsigned long long, __builtin_popcountll)
 // TODO: 128b specializations?
 #undef ADD_SPECIALIZATION
 
-template <typename T, typename = cpp::enable_if_t<cpp::is_unsigned_v<T>>>
-[[nodiscard]] LIBC_INLINE constexpr int count_zeros(T value) {
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
+count_zeros(T value) {
   return count_ones<T>(static_cast<T>(~value));
 }
 
diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index ae1fe7aaa18287..69cb0b5d1ce765 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -43,6 +43,9 @@ struct BigInt {
   static_assert(is_integral_v<WordType> && is_unsigned_v<WordType>,
                 "WordType must be unsigned integer.");
 
+  using word_type = WordType;
+  LIBC_INLINE_VAR static constexpr bool SIGNED = Signed;
+  LIBC_INLINE_VAR static constexpr size_t BITS = Bits;
   LIBC_INLINE_VAR
   static constexpr size_t WORD_SIZE = sizeof(WordType) * CHAR_BIT;
 
@@ -50,6 +53,10 @@ struct BigInt {
                 "Number of bits in BigInt should be a multiple of WORD_SIZE.");
 
   LIBC_INLINE_VAR static constexpr size_t WORD_COUNT = Bits / WORD_SIZE;
+
+  using unsigned_type = BigInt<BITS, false, word_type>;
+  using signed_type = BigInt<BITS, true, word_type>;
+
   cpp::array<WordType, WORD_COUNT> val{};
 
   LIBC_INLINE constexpr BigInt() = default;
@@ -579,19 +586,33 @@ struct BigInt {
     return *this;
   }
 
-  LIBC_INLINE constexpr uint64_t clz() {
-    uint64_t leading_zeroes = 0;
-    for (size_t i = WORD_COUNT; i > 0; --i) {
-      if (val[i - 1] == 0) {
-        leading_zeroes += WORD_SIZE;
-      } else {
-        leading_zeroes += countl_zero(val[i - 1]);
+  // TODO: remove and use cpp::countl_zero below.
+  [[nodiscard]] LIBC_INLINE constexpr int clz() const {
+    constexpr int word_digits = cpp::numeric_limits<word_type>::digits;
+    int leading_zeroes = 0;
+    for (auto i = val.size(); i > 0;) {
+      --i;
+      const int zeroes = countl_zero(val[i]);
+      leading_zeroes += zeroes;
+      if (zeroes != word_digits)
         break;
-      }
     }
     return leading_zeroes;
   }
 
+  // TODO: remove and use cpp::countr_zero below.
+  [[nodiscard]] LIBC_INLINE constexpr int ctz() const {
+    constexpr int word_digits = cpp::numeric_limits<word_type>::digits;
+    int trailing_zeroes = 0;
+    for (auto word : val) {
+      const int zeroes = countr_zero(word);
+      trailing_zeroes += zeroes;
+      if (zeroes != word_digits)
+        break;
+    }
+    return trailing_zeroes;
+  }
+
   LIBC_INLINE constexpr void shift_left(size_t s) {
     if constexpr (Bits == WORD_SIZE) {
       // Use native types if possible.
@@ -912,66 +933,169 @@ template <> class numeric_limits<Int<128>> {
   LIBC_INLINE_VAR static constexpr int digits = 128;
 };
 
-// Provides is_integral of U/Int<128>, U/Int<192>, U/Int<256>.
-template <size_t Bits, bool Signed, typename T>
-struct is_integral<BigInt<Bits, Signed, T>> : cpp::true_type {};
-
-// Provides is_unsigned of UInt<128>, UInt<192>, UInt<256>.
-template <size_t Bits, bool Signed, typename T>
-struct is_unsigned<BigInt<Bits, Signed, T>> : cpp::bool_constant<!Signed> {};
-
-template <size_t Bits, bool Signed, typename T>
-struct make_unsigned<BigInt<Bits, Signed, T>>
-    : type_identity<BigInt<Bits, false, T>> {};
-
-template <size_t Bits, bool Signed, typename T>
-struct make_signed<BigInt<Bits, Signed, T>>
-    : type_identity<BigInt<Bits, true, T>> {};
-
-namespace internal {
-template <typename T> struct is_custom_uint : cpp::false_type {};
+// type traits to determine whether a T is a cpp::BigInt.
+template <typename T> struct is_big_int : cpp::false_type {};
 
 template <size_t Bits, bool Signed, typename T>
-struct is_custom_uint<BigInt<Bits, Signed, T>> : cpp::true_type {};
-} // namespace internal
-
-// bit_cast to UInt
-// Note: The standard scheme for SFINAE selection is to have exactly one
-// function instanciation valid at a time. This is usually done by having a
-// predicate in one function and the negated predicate in the other one.
-// e.g.
-// template<typename = cpp::enable_if_t< is_custom_uint<To>::value == true> ...
-// template<typename = cpp::enable_if_t< is_custom_uint<To>::value == false> ...
-//
-// Unfortunately this would make the default 'cpp::bit_cast' aware of
-// 'is_custom_uint' (or any other customization). To prevent exposing all
-// customizations in the original function, we create a different function with
-// four 'typename's instead of three - otherwise it would be considered as a
-// redeclaration of the same function leading to "error: template parameter
-// redefines default argument".
-template <typename To, typename From,
-          typename = cpp::enable_if_t<sizeof(To) == sizeof(From) &&
-                                      cpp::is_trivially_copyable<To>::value &&
-                                      cpp::is_trivially_copyable<From>::value>,
-          typename = cpp::enable_if_t<internal::is_custom_uint<To>::value>>
-LIBC_INLINE constexpr To bit_cast(const From &from) {
+struct is_big_int<BigInt<Bits, Signed, T>> : cpp::true_type {};
+
+template <class T>
+LIBC_INLINE_VAR constexpr bool is_big_int_v = is_big_int<T>::value;
+
+// Specialization of cpp::bit_cast ('bit.h') from T to BigInt.
+template <typename To, typename From>
+LIBC_INLINE constexpr cpp::enable_if_t<
+    (sizeof(To) == sizeof(From)) && cpp::is_trivially_copyable<To>::value &&
+        cpp::is_trivially_copyable<From>::value && is_big_int<To>::value,
+    To>
+bit_cast(const From &from) {
   To out;
   using Storage = decltype(out.val);
   out.val = cpp::bit_cast<Storage>(from);
   return out;
 }
 
-// bit_cast from UInt
-template <
-    typename To, size_t Bits,
-    typename = cpp::enable_if_t<sizeof(To) == sizeof(UInt<Bits>) &&
-                                cpp::is_trivially_constructible<To>::value &&
-                                cpp::is_trivially_copyable<To>::value &&
-                                cpp::is_trivially_copyable<UInt<Bits>>::value>>
-LIBC_INLINE constexpr To bit_cast(const UInt<Bits> &from) {
+// Specialization of cpp::bit_cast ('bit.h') from BigInt to T.
+template <typename To, size_t Bits>
+LIBC_INLINE constexpr cpp::enable_if_t<
+    sizeof(To) == sizeof(UInt<Bits>) &&
+        cpp::is_trivially_constructible<To>::value &&
+        cpp::is_trivially_copyable<To>::value &&
+        cpp::is_trivially_copyable<UInt<Bits>>::value,
+    To>
+bit_cast(const UInt<Bits> &from) {
   return cpp::bit_cast<To>(from.val);
 }
 
+// Specialization of cpp::has_single_bit ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, bool>
+has_single_bit(T value) {
+  for (auto word : value.val)
+    if (cpp::has_single_bit(word))
+      return true;
+  return false;
+}
+
+// Specialization of cpp::countr_zero ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+countr_zero(const T &value) {
+  return value.ctz();
+}
+
+// Specialization of cpp::countl_zero ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+countl_zero(const T &value) {
+  return value.clz();
+}
+
+// Specialization of cpp::countl_one ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+countl_one(T value) {
+  // TODO : Implement a faster version.
+  return cpp::countl_zero<T>(~value);
+}
+
+// Specialization of cpp::countr_one ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+countr_one(T value) {
+  // TODO : Implement a faster version.
+  return cpp::countr_zero<T>(~value);
+}
+
+// Specialization of cpp::bit_width ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+bit_width(T value) {
+  return cpp::numeric_limits<T>::digits - cpp::countl_zero(value);
+}
+
+// Forward-declare rotr so that rotl can use it.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, T>
+rotr(T value, int rotate);
+
+// Specialization of cpp::rotl ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, T>
+rotl(T value, int rotate) {
+  // TODO : Implement a faster version.
+  constexpr unsigned N = cpp::numeric_limits<T>::digits;
+  rotate = rotate % N;
+  if (!rotate)
+    return value;
+  if (rotate < 0)
+    return cpp::rotr<T>(value, -rotate);
+  return (value << rotate) | (value >> (N - rotate));
+}
+
+// Specialization of cpp::rotr ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, T>
+rotr(T value, int rotate) {
+  // TODO : Implement a faster version.
+  constexpr unsigned N = cpp::numeric_limits<T>::digits;
+  rotate = rotate % N;
+  if (!rotate)
+    return value;
+  if (rotate < 0)
+    return cpp::rotl<T>(value, -rotate);
+  return (value >> rotate) | (value << (N - rotate));
+}
+
+// Specialization of cpp::first_leading_zero ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+first_leading_zero(T value) {
+  return value == cpp::numeric_limits<T>::max() ? 0 : countl_one(value) + 1;
+}
+
+// Specialization of cpp::first_leading_one ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+first_leading_one(T value) {
+  return first_leading_zero(static_cast<T>(~value));
+}
+
+// Specialization of cpp::first_trailing_zero ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+first_trailing_zero(T value) {
+  return value == cpp::numeric_limits<T>::max()
+             ? 0
+             : countr_zero(static_cast<T>(~value)) + 1;
+}
+
+// Specialization of cpp::first_trailing_one ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+first_trailing_one(T value) {
+  return value == cpp::numeric_limits<T>::max() ? 0 : countr_zero(value) + 1;
+}
+
+// Specialization of cpp::count_ones ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+count_ones(T value) {
+  // TODO : Implement a faster version.
+  int count = 0;
+  for (int i = 0; i != cpp::numeric_limits<T>::digits; ++i)
+    if ((value >> i) & 0x1)
+      ++count;
+  return count;
+}
+
+// Specialization of cpp::count_zeros ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+count_zeros(T value) {
+  return count_ones<T>(static_cast<T>(~value));
+}
+
 } // namespace LIBC_NAMESPACE::cpp
 
 #e...
[truncated]

lntue · 2024-03-05T16:22:23Z

libc/src/__support/UInt.h

@@ -43,13 +43,20 @@ struct BigInt {
  static_assert(is_integral_v<WordType> && is_unsigned_v<WordType>,
                "WordType must be unsigned integer.");

+  using word_type = WordType;


Will making these type aliases word_t, unsigned_t, and signed_t conflict with anything? B/c writing them all out makes them look like variable names.

Will making these type aliases word_t, unsigned_t, and signed_t conflict with anything?

It won't AFAICT.
In the cpp namespace we adopt the STL format so I sticked to the usual container's value_type.
Also TBH I would rather use element_type here instead of word_type but that would mean more changes.

libc/src/__support/UInt.h

libc/src/__support/integer_to_string.h

libc/src/__support/UInt.h

legrosbuffle · 2024-03-06T09:51:21Z

libc/src/__support/UInt.h

+// Specialization of cpp::first_leading_zero ('bit.h') for BigInt.
+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_big_int_v<T>, int>
+first_leading_zero(T value) {


Do we need these 4 functions ? AFAICT this is only used to implement C's first_leading_zero_*, which do not support BigInt.

Indeed, I've removed the non standard functions specialization.
@nickdesaulniers should we move the first_(leading|trailing)_(zero|one) and count_zeros functions outside the bit.h header? It feels to me that we should stick to standard functions in the CPP folder. How about moving them to math_extras.h? I've added a few comments in 48b0bc8

Yes, sorry, I should not have put them there. I'll work on cleaning that up.

Note: C23 added _BitInt which is a crazy type because you can have unsigned _BitInt(128) (or any value bitfield; I haven't read if there's a max width yet). So it may be helpful at some point to support these operations on 128 ints or bitints.

Yeah I've used _BitInt and _ExtInt before and there's one (important) caveat though : it works only up to uintmax_t so we can't implement BigInt with it. It would have been nice :)

…about non standard functions in the bit header.

lntue · 2024-03-06T16:20:26Z

libc/src/__support/CPP/bit.h

+template <typename T>
+[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>,
+                                                     bool>
+has_single_bit(T value) {


Just a side question: Is there a simple way to make these template functions be like:

has_single_bit(T value) when sizeof(T) <= 8 has_single_bit(const T &value) when sizeof(T) > 8

Especially if this pattern can be applied to many functions easily somehow.
Last time I experimented with DoubleDouble and DyadicFloat<128> and it did provide measurable performance improvements.

All our functions are inline. This means that the compiler can change how the value is passed to the function. That is, we don't care about passing by value or ref.

The only thing we need to make sure is that the type is [[trivial_abi]] (blog article), otherwise the compiler is forced to pass by ref to be able to call the destructor when there is one. This is true even if the signature of the function passes the argument by value. So technically we'd need to annotate BigInt and DoubleDouble with [[clang::trivial_abi]] (or its GCC equivalent). We can also static_assert<cpp::is_trivial_v<T>> for DoubleDouble and DyadicFloat<128>.

nickdesaulniers · 2024-03-06T16:42:22Z

libc/test/UnitTest/LibcTest.cpp

+cpp::enable_if_t<(cpp::is_integral_v<T> && (sizeof(T) > sizeof(uint64_t))) ||
+                     cpp::is_big_int_v<T>,


What's the difference between the LHS of the || and RHS? Does the LHS not hold for the RHS?

Quoting cpp_reference for std::is_integral:

Provides the member constant value which is equal to true, if T is the type bool, char, char8_t(since C++20), char16_t, char32_t, wchar_t, short, int, long, long long, or any implementation-defined extended integer types, including any signed, unsigned, and cv-qualified variants. Otherwise, value is equal to false.

If the program adds specializations for std::is_integral or std::is_integral_v, the behavior is undefined.

This means that BigInt is false for is_integral (that's the purpose of this patch)
This also means that __int128_t / __uint128_t is true.

So cpp::is_integral_v<T> && (sizeof(T) > sizeof(uint64_t))) means __int128_t or __uint128_t.
And cpp::is_big_int_v<T> means BigInt instantiation.

And cpp::is_big_int_v means BigInt instantiation.

Ah!

(that's the purpose of this patch)

Ok, now I see how it all fits together. Sorry for being slow on the uptake, but I do appreciate the patient explanations!

There's no equivalent to is_big_int_v in the std:: namespace. I was under the impression that our cpp:: is meant to mirror things in std::? As that was the intent behind these todos added in 48b0bc8. If those should be moved out of bit.h, should they (and cpp::is_big_int<T>) also be moved out of the cpp:: namespace?

https://libc.llvm.org/dev/code_style.html#setting-errno-from-runtime-code was the only reference I found to any kind of cpp:: convention; you would know better than I what our project's intent is with cpp::.

Or, perhaps am I conflating what does/does not belong under libc/src/__support/CPP with what does/does not belong in the cpp:: namespace?

Ok, now I see how it all fits together. Sorry for being slow on the uptake, but I do appreciate the patient explanations!

No worries! It's important to speak up when something is unclear, and I'm happy to provide explanations or discuss rationale earlier than later.

should they (and cpp::is_big_int) also be moved out of the cpp:: namespace?

I think so, there is a bit of cleanup left. I'll take care of it. Things have grown organically for a while and we're slowly paying the technical debt.

Ok, I'll work on finishing cleaning up the todos from 48b0bc8 next week.

…84035)" This reverts commit 84f483d.

…#84297) Reverts #84035 Several bots are failing: - https://lab.llvm.org/buildbot/#/builders/223/builds/37522 - https://lab.llvm.org/buildbot/#/builders/162/builds/51978 - https://lab.llvm.org/buildbot/#/builders/163/builds/52560 - https://lab.llvm.org/buildbot/#/builders/250/builds/19619

) The standard specifies that it it UB to specialize the following traits: - `std::is_integral` - `std::is_unsigned` - `std::make_unsigned` - `std::make_signed` This patch: - Removes specializations for `BigInt` - Transforms SFINAE for `bit.h` functions from template parameter to return type (This makes specialization easier). - Adds `BigInt` specialization for `bit.h` functions. - Fixes code depending on previous specializations.

…84299) Note: This is a reland of #84035. The standard specifies that it it UB to specialize the following traits: - `std::is_integral` - `std::is_unsigned` - `std::make_unsigned` - `std::make_signed` This patch: - Removes specializations for `BigInt` - Transforms SFINAE for `bit.h` functions from template parameter to return type (This makes specialization easier). - Adds `BigInt` specialization for `bit.h` functions. - Fixes code depending on previous specializations.

As noted in llvm#84035 (comment) only files under the CPP folder should be in the `cpp` namespace.

As noted in #84035 (comment) only files under the CPP folder should be in the `cpp` namespace.

llvmbot added the libc label Mar 5, 2024

gchatelet requested a review from legrosbuffle March 5, 2024 16:05

gchatelet requested a review from nickdesaulniers March 5, 2024 16:06

gchatelet changed the title ~~[libc] Remove UB specializations of type_traits~~ [libc] Remove UB specializations of type traits for BigInt Mar 5, 2024

lntue reviewed Mar 5, 2024

View reviewed changes

nickdesaulniers reviewed Mar 5, 2024

View reviewed changes

libc/src/__support/UInt.h Outdated Show resolved Hide resolved

libc/src/__support/integer_to_string.h Show resolved Hide resolved

libc/src/__support/UInt.h Outdated Show resolved Hide resolved

libc/src/__support/UInt.h Show resolved Hide resolved

Fix bug on has_single_bit

ccabea0

legrosbuffle reviewed Mar 6, 2024

View reviewed changes

gchatelet added 2 commits March 6, 2024 09:54

implement faster count_ones count_zeros, add documentation

71a1599

Remove non standard bit functions specializations, add documentation …

48b0bc8

…about non standard functions in the bit header.

gchatelet requested review from legrosbuffle, lntue and nickdesaulniers March 6, 2024 10:12

lntue reviewed Mar 6, 2024

View reviewed changes

nickdesaulniers approved these changes Mar 6, 2024

View reviewed changes

gchatelet merged commit 84f483d into llvm:main Mar 7, 2024
4 checks passed

gchatelet deleted the use_return_type_sfinae_for_bit_utils branch March 7, 2024 10:01

gchatelet added a commit that referenced this pull request Mar 7, 2024

Revert "[libc] Remove UB specializations of type traits for BigInt (#…

f3ca656

…84035)" This reverts commit 84f483d.

gchatelet mentioned this pull request Mar 7, 2024

Revert "[libc] Remove UB specializations of type traits for BigInt" #84297

Merged

gchatelet mentioned this pull request Mar 7, 2024

[reland][libc] Remove UB specializations of type traits for BigInt #84299

Merged

gchatelet added a commit to gchatelet/llvm-project that referenced this pull request Mar 8, 2024

[libc][NFC] Move BigInt out of the cpp namespace

9b46147

As noted in llvm#84035 (comment) only files under the CPP folder should be in the `cpp` namespace.

gchatelet mentioned this pull request Mar 8, 2024

[libc][NFC] Move BigInt out of the cpp namespace #84445

Merged

gchatelet added a commit that referenced this pull request Mar 8, 2024

[libc][NFC] Move BigInt out of the cpp namespace (#84445)

6a8e6c9

As noted in #84035 (comment) only files under the CPP folder should be in the `cpp` namespace.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc] Remove UB specializations of type traits for `BigInt` #84035

[libc] Remove UB specializations of type traits for `BigInt` #84035

gchatelet commented Mar 5, 2024 •

edited

Loading

llvmbot commented Mar 5, 2024

lntue Mar 5, 2024

gchatelet Mar 6, 2024

legrosbuffle Mar 6, 2024

gchatelet Mar 6, 2024

nickdesaulniers Mar 6, 2024

nickdesaulniers Mar 6, 2024

gchatelet Mar 7, 2024 •

edited

Loading

lntue Mar 6, 2024

gchatelet Mar 7, 2024

nickdesaulniers Mar 6, 2024

gchatelet Mar 7, 2024

nickdesaulniers Mar 7, 2024

gchatelet Mar 8, 2024

nickdesaulniers Mar 8, 2024

		cpp::enable_if_t<(cpp::is_integral_v<T> && (sizeof(T) > sizeof(uint64_t))) \|\|
		cpp::is_big_int_v<T>,

[libc] Remove UB specializations of type traits for BigInt #84035

[libc] Remove UB specializations of type traits for BigInt #84035

Conversation

gchatelet commented Mar 5, 2024 • edited Loading

llvmbot commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gchatelet Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[libc] Remove UB specializations of type traits for `BigInt` #84035

[libc] Remove UB specializations of type traits for `BigInt` #84035

gchatelet commented Mar 5, 2024 •

edited

Loading

gchatelet Mar 7, 2024 •

edited

Loading