Add Int<> and Int128 types to replace the usage of int128_t in math
functions. Clean up to make sure that (U)Int128 and (u)int128_t are
interchangeable in the code base.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/test/src/math/TruncTest.h | ||
---|---|---|
68 ↗ | (On Diff #529669) | I did not do a detailed review but why were these constexpr decls converted to const decls? |
libc/test/src/math/TruncTest.h | ||
---|---|---|
68 ↗ | (On Diff #529669) | Because these use the division operator, which using clz() internally, which in turn calls __builtin_clz* that are not constexpr. |
libc/test/src/math/TruncTest.h | ||
---|---|---|
68 ↗ | (On Diff #529669) | We should be able to implement cpp::is_constant_evaluated using __builtin_is_constant_evaluated and use it to retain the constexpr nature where relevant. We should ideally do that first before adding BigInt. |
libc/test/src/math/TruncTest.h | ||
---|---|---|
68 ↗ | (On Diff #529669) | Turns out that __builtin_clz* are already constexpr, so marking all our clz functions constexpr is enough. |
overall LGTM
libc/src/__support/UInt.h | ||
---|---|---|
77 | would it be possible to add a template version of this initializer for the primitive uint128? |
Add conversion to and from __(u)int128_t.
libc/src/__support/UInt.h | ||
---|---|---|
77 | I added conversion to and from __(u)int128_t. |
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h | ||
---|---|---|
137 | In https://reviews.llvm.org/D149594, I had to set the explicit bit when the value is not zero because some tests start to fail due to: diff --git a/libc/src/__support/str_to_float.h b/libc/src/__support/str_to_float.h index b36c73fbe8ef..79a1c8fe6f0b 100644 --- a/libc/src/__support/str_to_float.h +++ b/libc/src/__support/str_to_float.h @@ -13,6 +13,7 @@ #include "src/__support/CPP/optional.h" #include "src/__support/FPUtil/FEnvImpl.h" #include "src/__support/FPUtil/FPBits.h" +#include "src/__support/FPUtil/dyadic_float.h" #include "src/__support/UInt128.h" #include "src/__support/builtin_wrappers.h" #include "src/__support/common.h" @@ -570,7 +572,9 @@ clinger_fast_path(ExpandedFloat<T> init_num, } fputil::FPBits<T> result; - T float_mantissa = static_cast<T>(mantissa); + T float_mantissa = + static_cast<T>(fputil::DyadicFloat<128>(/*sign*/ false, /*exponent*/ 0, + /*mantissa*/ mantissa)); if (exp10 == 0) { result = fputil::FPBits<T>(float_mantissa); -- 2.39.2 We need this change to support platforms with no native 128-bit integers. May I ask if you can double-check that? |
The tests are all passing in rv32 once we apply D148797, D150211, D150223 and the following change:
diff --git a/libc/src/__support/FPUtil/generic/sqrt.h b/libc/src/__support/FPUtil/generic/sqrt.h index 9e9896ed185f..7c4e4a485320 100644 --- a/libc/src/__support/FPUtil/generic/sqrt.h +++ b/libc/src/__support/FPUtil/generic/sqrt.h @@ -137,7 +137,7 @@ LIBC_INLINE cpp::enable_if_t<cpp::is_floating_point_v<T>, T> sqrt(T x) { } // We compute one more iteration in order to round correctly. - bool lsb = y & 1; // Least significant bit + bool lsb = bool(y & 1); // Least significant bit bool rb = false; // Round bit r <<= 2; UIntType tmp = (y << 2) + 1;
Feel free to add it to this patch, otherwise, I can add it to D150223 once I rebase it.
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h | ||
---|---|---|
137 | This code path should not be in ARM32 or RISCV-32 platforms? I thought that for those platform, long double is either double or quad precision (Float128), right? |
libc/src/__support/UInt.h | ||
---|---|---|
89 | Should there be an additional condition sizeof(T) <= 16? |
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h | ||
---|---|---|
137 | It's not used in rv32, but because of the change to the clinger_fast_path function, this gets called in other platforms |
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h | ||
---|---|---|
137 | Do you mind creating a github issue for me or @michaelrj to investigate the problem? It's possible that we might need to clean up some comparison / testing codes for x86 long double instead. |
In https://reviews.llvm.org/D149594, I had to set the explicit bit when the value is not zero because some tests start to fail due to:
We need this change to support platforms with no native 128-bit integers.
May I ask if you can double-check that?