[libc] Make BigInt bit_cast-able to compatible types #75063
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a second take on llvm#74837 to fix llvm#74258
|
@llvm/pr-subscribers-libc Author: Guillaume Chatelet (gchatelet) ChangesThis is a second take on #74837 to fix #74258 Full diff: https://github.com/llvm/llvm-project/pull/75063.diff 5 Files Affected:
diff --git a/libc/src/__support/CPP/bit.h b/libc/src/__support/CPP/bit.h
index 2091c3662d507..f9bb2ef9f8332 100644
--- a/libc/src/__support/CPP/bit.h
+++ b/libc/src/__support/CPP/bit.h
@@ -25,6 +25,27 @@ namespace LIBC_NAMESPACE::cpp {
#define LLVM_LIBC_HAS_BUILTIN_MEMCPY_INLINE
#endif
+// Performs a copy from 'src' to 'dst' of 'size' bytes.
+// The semantics is valid if 'src' and 'dst' are equal but undefined if the
+// regions defined by [src, src + size] and [dst, dst + size] overlap.
+template <size_t size, typename DstT, typename SrcT>
+LIBC_INLINE constexpr void memcpy_inline(DstT *__restrict dst,
+ const SrcT *__restrict src) {
+#ifdef LLVM_LIBC_HAS_BUILTIN_MEMCPY_INLINE
+ __builtin_memcpy(dst, src, size);
+#else
+ // `memcpy_inline` is instantiated several times with different value of the
+ // size parameter. This doesn't play well with GCC's Value Range Analysis that
+ // wrongly detects out of bounds accesses. We disable the 'array-bounds'
+ // warning for the purpose of this function.
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+ for (size_t i = 0; i < size; ++i)
+ static_cast<char *>(dst)[i] = static_cast<const char *>(src)[i];
+#pragma GCC diagnostic pop
+#endif
+}
+
// This implementation of bit_cast requires trivially-constructible To, to avoid
// UB in the implementation.
template <
@@ -39,14 +60,7 @@ LIBC_INLINE constexpr To bit_cast(const From &from) {
return __builtin_bit_cast(To, from);
#else
To to;
- char *dst = reinterpret_cast<char *>(&to);
- const char *src = reinterpret_cast<const char *>(&from);
-#if LIBC_HAS_BUILTIN(__builtin_memcpy_inline)
- __builtin_memcpy_inline(dst, src, sizeof(To));
-#else
- for (unsigned i = 0; i < sizeof(To); ++i)
- dst[i] = src[i];
-#endif // LIBC_HAS_BUILTIN(__builtin_memcpy_inline)
+ memcpy_inline<sizeof(To)>(&to, &from);
return to;
#endif // LIBC_HAS_BUILTIN(__builtin_bit_cast)
}
diff --git a/libc/src/__support/FPUtil/FPBits.h b/libc/src/__support/FPUtil/FPBits.h
index ca98aa7126249..eb75abc379a03 100644
--- a/libc/src/__support/FPUtil/FPBits.h
+++ b/libc/src/__support/FPUtil/FPBits.h
@@ -11,6 +11,7 @@
#include "src/__support/CPP/bit.h"
#include "src/__support/CPP/type_traits.h"
+#include "src/__support/UInt128.h"
#include "src/__support/common.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE
diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index f72b995f8788d..78d8213cabcf9 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -952,6 +952,35 @@ struct make_signed<UInt<Bits>> : type_identity<Int<Bits>> {
"Number of bits in Int should be a multiple of 64.");
};
+namespace internal {
+template <typename T> struct is_custom_uint : cpp::false_type {};
+template <size_t Bits> struct is_custom_uint<UInt<Bits>> : cpp::true_type {};
+} // namespace internal
+
+// bit_cast to UInt
+template <typename To, typename From,
+ typename = cpp::enable_if_t<internal::is_custom_uint<To>::value>,
+ typename = cpp::enable_if_t<sizeof(To) == sizeof(From)>,
+ typename = cpp::enable_if_t<cpp::is_trivially_copyable<From>::value>>
+LIBC_INLINE constexpr To bit_cast(const From &from) {
+ To out;
+ cpp::memcpy_inline<sizeof(out)>(out.val, &from);
+ return out;
+}
+
+// bit_cast from UInt
+template <
+ typename To, size_t Bits,
+ typename = cpp::enable_if_t<sizeof(To) == sizeof(UInt<Bits>)>,
+ typename = cpp::enable_if_t<cpp::is_trivially_constructible<To>::value>,
+ typename = cpp::enable_if_t<cpp::is_trivially_copyable<To>::value>,
+ typename = cpp::enable_if_t<cpp::is_trivially_copyable<UInt<Bits>>::value>>
+LIBC_INLINE constexpr To bit_cast(const UInt<Bits> &from) {
+ To out;
+ cpp::memcpy_inline<sizeof(out)>(&out, from.val);
+ return out;
+}
+
} // namespace LIBC_NAMESPACE::cpp
#endif // LLVM_LIBC_SRC___SUPPORT_UINT_H
diff --git a/libc/src/string/memory_utils/utils.h b/libc/src/string/memory_utils/utils.h
index 5cd716e033d6a..bbc9f68edb4bd 100644
--- a/libc/src/string/memory_utils/utils.h
+++ b/libc/src/string/memory_utils/utils.h
@@ -71,33 +71,10 @@ LIBC_INLINE bool is_disjoint(const void *p1, const void *p2, size_t size) {
return sdiff >= 0 ? size <= udiff : size <= neg_udiff;
}
-#if LIBC_HAS_BUILTIN(__builtin_memcpy_inline)
-#define LLVM_LIBC_HAS_BUILTIN_MEMCPY_INLINE
-#endif
-
#if LIBC_HAS_BUILTIN(__builtin_memset_inline)
#define LLVM_LIBC_HAS_BUILTIN_MEMSET_INLINE
#endif
-// Performs a constant count copy.
-template <size_t Size>
-LIBC_INLINE void memcpy_inline(void *__restrict dst,
- const void *__restrict src) {
-#ifdef LLVM_LIBC_HAS_BUILTIN_MEMCPY_INLINE
- __builtin_memcpy_inline(dst, src, Size);
-#else
- // In memory functions `memcpy_inline` is instantiated several times with
- // different value of the Size parameter. This doesn't play well with GCC's
- // Value Range Analysis that wrongly detects out of bounds accesses. We
- // disable the 'array-bounds' warning for the purpose of this function.
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Warray-bounds"
- for (size_t i = 0; i < Size; ++i)
- static_cast<char *>(dst)[i] = static_cast<const char *>(src)[i];
-#pragma GCC diagnostic pop
-#endif
-}
-
using Ptr = cpp::byte *; // Pointer to raw data.
using CPtr = const cpp::byte *; // Const pointer to raw data.
@@ -204,13 +181,13 @@ LIBC_INLINE MemcmpReturnType cmp_neq_uint64_t(uint64_t a, uint64_t b) {
// type.
template <typename T> LIBC_INLINE T load(CPtr ptr) {
T Out;
- memcpy_inline<sizeof(T)>(&Out, ptr);
+ cpp::memcpy_inline<sizeof(T)>(&Out, ptr);
return Out;
}
// Stores a value of type T in memory (possibly unaligned).
template <typename T> LIBC_INLINE void store(Ptr ptr, T value) {
- memcpy_inline<sizeof(T)>(ptr, &value);
+ cpp::memcpy_inline<sizeof(T)>(ptr, &value);
}
// On architectures that do not allow for unaligned access we perform several
diff --git a/libc/test/src/__support/uint_test.cpp b/libc/test/src/__support/uint_test.cpp
index 971bac55bd9d3..0ad72c35645c4 100644
--- a/libc/test/src/__support/uint_test.cpp
+++ b/libc/test/src/__support/uint_test.cpp
@@ -10,19 +10,62 @@
#include "src/__support/UInt.h"
#include "test/UnitTest/Test.h"
+#include <math.h> // HUGE_VALF, HUGE_VALF
-// We want to test LIBC_NAMESPACE::cpp::UInt<128> explicitly. So, for
+namespace LIBC_NAMESPACE {
+
+using LL_UInt64 = cpp::UInt<64>;
+// We want to test cpp::UInt<128> explicitly. So, for
// convenience, we use a sugar which does not conflict with the UInt128 type
// which can resolve to __uint128_t if the platform has it.
-using LL_UInt128 = LIBC_NAMESPACE::cpp::UInt<128>;
-using LL_UInt192 = LIBC_NAMESPACE::cpp::UInt<192>;
-using LL_UInt256 = LIBC_NAMESPACE::cpp::UInt<256>;
-using LL_UInt320 = LIBC_NAMESPACE::cpp::UInt<320>;
-using LL_UInt512 = LIBC_NAMESPACE::cpp::UInt<512>;
-using LL_UInt1024 = LIBC_NAMESPACE::cpp::UInt<1024>;
+using LL_UInt128 = cpp::UInt<128>;
+using LL_UInt192 = cpp::UInt<192>;
+using LL_UInt256 = cpp::UInt<256>;
+using LL_UInt320 = cpp::UInt<320>;
+using LL_UInt512 = cpp::UInt<512>;
+using LL_UInt1024 = cpp::UInt<1024>;
+
+using LL_Int128 = cpp::Int<128>;
+using LL_Int192 = cpp::Int<192>;
+
+TEST(LlvmLibcUIntClassTest, BitCastToFromDouble) {
+ static_assert(cpp::is_trivially_copyable<LL_UInt64>::value);
+ static_assert(sizeof(LL_UInt64) == sizeof(double));
+ const double inf = HUGE_VAL;
+ const double max = DBL_MAX;
+ const double array[] = {0.0, 0.1, 1.0, max, inf};
+ for (double value : array) {
+ LL_UInt64 back = cpp::bit_cast<LL_UInt64>(value);
+ double forth = cpp::bit_cast<double>(back);
+ EXPECT_TRUE(value == forth);
+ }
+}
-using LL_Int128 = LIBC_NAMESPACE::cpp::Int<128>;
-using LL_Int192 = LIBC_NAMESPACE::cpp::Int<192>;
+#ifdef __SIZEOF_INT128__
+TEST(LlvmLibcUIntClassTest, BitCastToFromNativeUint128) {
+ static_assert(cpp::is_trivially_copyable<LL_UInt128>::value);
+ static_assert(sizeof(LL_UInt128) == sizeof(__uint128_t));
+ const __uint128_t array[] = {0, 1, ~__uint128_t(0)};
+ for (__uint128_t value : array) {
+ LL_UInt128 back = cpp::bit_cast<LL_UInt128>(value);
+ __uint128_t forth = cpp::bit_cast<__uint128_t>(back);
+ EXPECT_TRUE(value == forth);
+ }
+}
+#endif
+
+#ifdef LIBC_COMPILER_HAS_FLOAT128
+TEST(LlvmLibcUIntClassTest, BitCastToFromNativeFloat128) {
+ static_assert(cpp::is_trivially_copyable<LL_UInt128>::value);
+ static_assert(sizeof(LL_UInt128) == sizeof(float128));
+ const float128 array[] = {0, 0.1, 1};
+ for (float128 value : array) {
+ LL_UInt128 back = cpp::bit_cast<LL_UInt128>(value);
+ float128 forth = cpp::bit_cast<float128>(back);
+ EXPECT_TRUE(value == forth);
+ }
+}
+#endif
TEST(LlvmLibcUIntClassTest, BasicInit) {
LL_UInt128 half_val(12345);
@@ -634,3 +677,5 @@ TEST(LlvmLibcUIntClassTest, ConstructorFromUInt128Tests) {
}
#endif // __SIZEOF_INT128__
+
+} // namespace LIBC_NAMESPACE
|
gchatelet
commented
Dec 11, 2023
|
Contrary to #74837 this patch does not try to make the type |
|
@michaelrj-google @lntue @petrhosek Now even with this patch in, when we disable native support for 128 bit uint types in llvm-project/libc/src/__support/UInt128.h Lines 14 to 20 in a9adcef and use UInt128 = cpp::UInt<128>, we have the following error:
I tried to trace it down but it feels like an issue with the |
|
The recent breakage on the arm32 build bot might also be related to how https://lab.llvm.org/buildbot/#/builders/229/builds/20953/steps/6/logs/stdio This fails during Intel Extended Precision instantiation which requires 128 bit types. ARM32 does not have edit: I can reproduce the error on x86 by manually forcing edit: Initializing an unsigned bigint with edit: Fixed in #75084 although the failure in #75063 (comment) is still here. |
lntue
reviewed
Dec 18, 2023
gchatelet
requested review from
legrosbuffle
and removed request for
legrosbuffle
legrosbuffle
approved these changes
Dec 20, 2023
lntue
approved these changes
Dec 20, 2023
michaelrj-google
approved these changes
Dec 20, 2023
legrosbuffle
approved these changes
Dec 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a second take on #74837 to fix #74258