This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add Int<> type and fix (U)Int<128> compatibility issues.
ClosedPublic

Authored by lntue on Jun 8 2023, 10:29 AM.

Details

Summary

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.

Diff Detail

Event Timeline

lntue created this revision.Jun 8 2023, 10:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 8 2023, 10:29 AM
lntue requested review of this revision.Jun 8 2023, 10:29 AM
sivachandra added inline comments.Jun 8 2023, 12:05 PM
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?

lntue added inline comments.Jun 8 2023, 1:55 PM
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.

lntue updated this revision to Diff 529737.Jun 8 2023, 3:01 PM

Add missing dependencies.

sivachandra added inline comments.Jun 9 2023, 1:47 AM
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.

lntue updated this revision to Diff 529950.Jun 9 2023, 6:58 AM

Make clz functions constexpr.

lntue updated this revision to Diff 529952.Jun 9 2023, 7:02 AM

Revert LDExpTest changes and sync to HEAD.

lntue added inline comments.Jun 9 2023, 7:21 AM
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.

lntue updated this revision to Diff 529957.Jun 9 2023, 7:22 AM

Fix LibcTest merging issue.

overall LGTM

libc/src/__support/UInt.h
77

would it be possible to add a template version of this initializer for the primitive uint128?

lntue updated this revision to Diff 530129.Jun 9 2023, 6:33 PM

Add conversion to and from __(u)int128_t.

libc/src/__support/UInt.h
77

I added conversion to and from __(u)int128_t.

lntue updated this revision to Diff 530494.Jun 12 2023, 6:50 AM

Fix a unit test and sync to HEAD.

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?

mikhail.ramalho accepted this revision.Jun 12 2023, 2:00 PM

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.

This revision is now accepted and ready to land.Jun 12 2023, 2:00 PM
lntue added inline comments.Jun 12 2023, 9:31 PM
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?

sivachandra accepted this revision.Jun 12 2023, 10:43 PM
sivachandra added inline comments.
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

lntue marked an inline comment as done.Jun 13 2023, 6:40 AM
lntue added inline comments.
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.

This revision was landed with ongoing or failed builds.Jun 13 2023, 6:40 AM
This revision was automatically updated to reflect the committed changes.