Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc][math][c23] adds nanf128 #85201

Merged
merged 7 commits into from
Mar 15, 2024
Merged

[libc][math][c23] adds nanf128 #85201

merged 7 commits into from
Mar 15, 2024

Conversation

Flandini
Copy link
Contributor

@Flandini Flandini commented Mar 14, 2024

Continuing #84689, this one required more changes than the others, so I am making it a separate PR.

Extends some stuff in str_to_float.h, str_to_integer.h to work on types wider than unsigned long long and uint64_t.

This also edits UInt.h to add type traits for make_unsigned and make_signed since make_unsigned<UInt<128>> is used in strtointeger now. I also added the other cases of make_unsigned and make_signed for BigInt for completeness. See review comments below.

I think the float128, and other code making use of UInt.h, are only being tested with LIBC_TYPES_HAS_INT128 defined. When I removed the define of LIBC_TYPES_HAS_INT128 in src/__support/macros/properties/types.h, I got a lot of compiler errors. I think this is because many type traits are not defined for BigInts, only for __uint128_t and __int128_t. is_unsigned is one type trait that is not defined for BigInts. I am also unsure if this is an issue or if I am just not 'not defining' LIBC_TYPES_HAS_INT128 properly. If this is an issue, I also wonder if we should be testing both sides of the preprocessor condition in UInt128.h. Let me know if I should open an issue.

cc @lntue for review.

@llvmbot llvmbot added the libc label Mar 14, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 14, 2024

@llvm/pr-subscribers-libc

Author: Michael Flanders (Flandini)

Changes

Continuing #84689, this one required more changes than the others, so I am making it a separate PR.

Extends some stuff in str_to_float.h, str_to_integer.h to work on types wider than unsigned long long and uint64_t.

This also edits UInt.h to add type traits for make_unsigned and make_signed since make_unsigned&lt;UInt&lt;128&gt;&gt; is used in strtointeger now. I also added the other cases of make_unsigned and make_signed for BigInt for completeness.

I think the float128, and other code making use of UInt.h, are only being tested with LIBC_TYPES_HAS_INT128 defined. When I removed the define of LIBC_TYPES_HAS_INT128 in src/__support/macros/properties/types.h, I got a lot of compiler errors. I think this is because many type traits are not defined for BigInts, only for __uint128_t and __int128_t. is_unsigned is one type trait that is not defined for BigInts. I am also unsure if this is an issue or if I am just not 'not defining' LIBC_TYPES_HAS_INT128 properly. If this is an issue, I also wonder if we should be testing both sides of the preprocessor condition in UInt128.h. Let me know if I should open an issue.

cc @lntue for review.


Full diff: https://github.com/llvm/llvm-project/pull/85201.diff

13 Files Affected:

  • (modified) libc/config/linux/aarch64/entrypoints.txt (+1)
  • (modified) libc/config/linux/riscv/entrypoints.txt (+1)
  • (modified) libc/config/linux/x86_64/entrypoints.txt (+1)
  • (modified) libc/spec/stdc.td (+1)
  • (modified) libc/src/__support/UInt.h (+9)
  • (modified) libc/src/__support/str_to_float.h (+11-15)
  • (modified) libc/src/__support/str_to_integer.h (+15-9)
  • (modified) libc/src/math/CMakeLists.txt (+1)
  • (modified) libc/src/math/generic/CMakeLists.txt (+13)
  • (added) libc/src/math/generic/nanf128.cpp (+23)
  • (added) libc/src/math/nanf128.h (+20)
  • (modified) libc/test/src/math/smoke/CMakeLists.txt (+16)
  • (added) libc/test/src/math/smoke/nanf128_test.cpp (+64)
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index 7d69099e3cb9db..43c9e81f17833e 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -468,6 +468,7 @@ if(LIBC_TYPES_HAS_FLOAT128)
     libc.src.math.lrintf128
     libc.src.math.lroundf128
     libc.src.math.modff128
+    libc.src.math.nanf128
     libc.src.math.nextafterf128
     libc.src.math.rintf128
     libc.src.math.roundf128
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index b1c9dd0428eea5..99ef84d3f73974 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -476,6 +476,7 @@ if(LIBC_TYPES_HAS_FLOAT128)
     libc.src.math.lrintf128
     libc.src.math.lroundf128
     libc.src.math.modff128
+    libc.src.math.nanf128
     libc.src.math.nextafterf128
     libc.src.math.rintf128
     libc.src.math.roundf128
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 4fb31c593b9dc7..a8b416aa9a0cda 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -481,6 +481,7 @@ if(LIBC_TYPES_HAS_FLOAT128)
     libc.src.math.lrintf128
     libc.src.math.lroundf128
     libc.src.math.modff128
+    libc.src.math.nanf128
     libc.src.math.nextafterf128
     libc.src.math.rintf128
     libc.src.math.roundf128
diff --git a/libc/spec/stdc.td b/libc/spec/stdc.td
index afe01b1bb68566..2bc9bc8b9b1a6f 100644
--- a/libc/spec/stdc.td
+++ b/libc/spec/stdc.td
@@ -559,6 +559,7 @@ def StdC : StandardSpec<"stdc"> {
           FunctionSpec<"nanf", RetValSpec<FloatType>, [ArgSpec<ConstCharPtr>]>,
           FunctionSpec<"nan", RetValSpec<DoubleType>, [ArgSpec<ConstCharPtr>]>,
           FunctionSpec<"nanl", RetValSpec<LongDoubleType>, [ArgSpec<ConstCharPtr>]>,
+          GuardedFunctionSpec<"nanf128", RetValSpec<Float128Type>, [ArgSpec<ConstCharPtr>], "LIBC_TYPES_HAS_FLOAT128">,
       ]
   >;
 
diff --git a/libc/src/__support/UInt.h b/libc/src/__support/UInt.h
index d92d61ed094ebe..97ecfe8606dbfb 100644
--- a/libc/src/__support/UInt.h
+++ b/libc/src/__support/UInt.h
@@ -993,6 +993,15 @@ 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;
 
+// make_unsigned and make_signed type traits
+template <size_t Bits, bool Signed, typename T>
+struct cpp::make_unsigned<BigInt<Bits, Signed, T>>
+    : cpp::type_identity<BigInt<Bits, false, T>> {};
+
+template <size_t Bits, bool Signed, typename T>
+struct cpp::make_signed<BigInt<Bits, Signed, T>>
+    : cpp::type_identity<BigInt<Bits, true, T>> {};
+
 namespace cpp {
 
 // Specialization of cpp::bit_cast ('bit.h') from T to BigInt.
diff --git a/libc/src/__support/str_to_float.h b/libc/src/__support/str_to_float.h
index 073e1dc6721723..2cf2cfb027243e 100644
--- a/libc/src/__support/str_to_float.h
+++ b/libc/src/__support/str_to_float.h
@@ -1056,17 +1056,17 @@ hexadecimal_string_to_float(const char *__restrict src,
   return output;
 }
 
-LIBC_INLINE uint64_t
+template <class T>
+LIBC_INLINE typename fputil::FPBits<T>::StorageType
 nan_mantissa_from_ncharseq(const cpp::string_view ncharseq) {
-  uint64_t nan_mantissa = 0;
+  using FPBits = typename fputil::FPBits<T>;
+  using StorageType = typename FPBits::StorageType;
+
+  StorageType nan_mantissa = 0;
 
   if (ncharseq.data() != nullptr && isdigit(ncharseq[0])) {
-    // This is to prevent errors when StorageType is larger than 64
-    // bits, since strtointeger only supports up to 64 bits. This is
-    // actually more than is required by the specification, which says
-    // for the input type "NAN(n-char-sequence)" that "the meaning of
-    // the n-char sequence is implementation-defined."
-    auto strtoint_result = strtointeger<uint64_t>(ncharseq.data(), 0);
+    StrToNumResult<StorageType> strtoint_result =
+        strtointeger<StorageType>(ncharseq.data(), 0);
     if (!strtoint_result.has_error())
       nan_mantissa = strtoint_result.value;
 
@@ -1172,9 +1172,8 @@ LIBC_INLINE StrToNumResult<T> strtofloatingpoint(const char *__restrict src) {
           ++index;
         if (src[index] == ')') {
           ++index;
-          auto nan_mantissa_result = nan_mantissa_from_ncharseq(
+          nan_mantissa = nan_mantissa_from_ncharseq<T>(
               cpp::string_view(src + (left_paren + 1), index - left_paren - 2));
-          nan_mantissa = static_cast<StorageType>(nan_mantissa_result);
         } else {
           index = left_paren;
         }
@@ -1221,11 +1220,8 @@ template <class T> LIBC_INLINE StrToNumResult<T> strtonan(const char *arg) {
   while (isalnum(arg[index]) || arg[index] == '_')
     ++index;
 
-  if (arg[index] == '\0') {
-    auto nan_mantissa_result =
-        nan_mantissa_from_ncharseq(cpp::string_view(arg, index));
-    nan_mantissa = static_cast<StorageType>(nan_mantissa_result);
-  }
+  if (arg[index] == '\0')
+    nan_mantissa = nan_mantissa_from_ncharseq<T>(cpp::string_view(arg, index));
 
   result = FPBits::quiet_nan(fputil::Sign::POS, nan_mantissa);
   return {result.get_val(), 0, error};
diff --git a/libc/src/__support/str_to_integer.h b/libc/src/__support/str_to_integer.h
index b87808993fee50..85a46065203f17 100644
--- a/libc/src/__support/str_to_integer.h
+++ b/libc/src/__support/str_to_integer.h
@@ -11,6 +11,7 @@
 
 #include "src/__support/CPP/limits.h"
 #include "src/__support/CPP/type_traits.h"
+#include "src/__support/UInt128.h"
 #include "src/__support/common.h"
 #include "src/__support/ctype_utils.h"
 #include "src/__support/str_to_num_result.h"
@@ -75,8 +76,12 @@ template <class T>
 LIBC_INLINE StrToNumResult<T>
 strtointeger(const char *__restrict src, int base,
              const size_t src_len = cpp::numeric_limits<size_t>::max()) {
-  // TODO: Rewrite to support numbers longer than long long
-  unsigned long long result = 0;
+  using ResultType = typename cpp::conditional_t<(cpp::is_same_v<T, UInt128> ||
+                                                  cpp::is_same_v<T, Int128>),
+                                                 UInt128, unsigned long long>;
+
+  ResultType result = 0;
+
   bool is_number = false;
   size_t src_cur = 0;
   int error_val = 0;
@@ -101,15 +106,16 @@ strtointeger(const char *__restrict src, int base,
   if (base == 16 && is_hex_start(src + src_cur, src_len - src_cur))
     src_cur = src_cur + 2;
 
-  constexpr bool IS_UNSIGNED = (cpp::numeric_limits<T>::min() == 0);
+  constexpr bool IS_UNSIGNED = cpp::is_unsigned_v<T>;
   const bool is_positive = (result_sign == '+');
-  unsigned long long constexpr NEGATIVE_MAX =
-      !IS_UNSIGNED
-          ? static_cast<unsigned long long>(cpp::numeric_limits<T>::max()) + 1
-          : cpp::numeric_limits<T>::max();
-  unsigned long long const abs_max =
+
+  ResultType constexpr NEGATIVE_MAX =
+      !IS_UNSIGNED ? static_cast<ResultType>(cpp::numeric_limits<T>::max()) + 1
+                   : cpp::numeric_limits<T>::max();
+  ResultType const abs_max =
       (is_positive ? cpp::numeric_limits<T>::max() : NEGATIVE_MAX);
-  unsigned long long const abs_max_div_by_base = abs_max / base;
+  ResultType const abs_max_div_by_base = abs_max / base;
+
   while (src_cur < src_len && isalnum(src[src_cur])) {
     int cur_digit = b36_char_to_int(src[src_cur]);
     if (cur_digit >= base)
diff --git a/libc/src/math/CMakeLists.txt b/libc/src/math/CMakeLists.txt
index 750fd5f0e3a9ba..cd03065399480e 100644
--- a/libc/src/math/CMakeLists.txt
+++ b/libc/src/math/CMakeLists.txt
@@ -190,6 +190,7 @@ add_math_entrypoint_object(modff128)
 add_math_entrypoint_object(nan)
 add_math_entrypoint_object(nanf)
 add_math_entrypoint_object(nanl)
+add_math_entrypoint_object(nanf128)
 
 add_math_entrypoint_object(nearbyint)
 add_math_entrypoint_object(nearbyintf)
diff --git a/libc/src/math/generic/CMakeLists.txt b/libc/src/math/generic/CMakeLists.txt
index 667381d615d1e0..87f53105a1b317 100644
--- a/libc/src/math/generic/CMakeLists.txt
+++ b/libc/src/math/generic/CMakeLists.txt
@@ -1780,6 +1780,19 @@ add_entrypoint_object(
     -O3
 )
 
+add_entrypoint_object(
+  nanf128
+  SRCS
+    nanf128.cpp
+  HDRS
+    ../nanf128.h
+  DEPENDS
+    libc.src.__support.str_to_float
+    libc.src.errno.errno
+  COMPILE_OPTIONS
+    -O3
+)
+
 add_entrypoint_object(
   nextafter
   SRCS
diff --git a/libc/src/math/generic/nanf128.cpp b/libc/src/math/generic/nanf128.cpp
new file mode 100644
index 00000000000000..f087c9f074fde8
--- /dev/null
+++ b/libc/src/math/generic/nanf128.cpp
@@ -0,0 +1,23 @@
+//===-- Implementation of nanf128 function --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/math/nanf128.h"
+#include "src/__support/common.h"
+#include "src/__support/str_to_float.h"
+#include "src/errno/libc_errno.h"
+
+namespace LIBC_NAMESPACE {
+
+LLVM_LIBC_FUNCTION(float128, nanf128, (const char *arg)) {
+  auto result = internal::strtonan<float128>(arg);
+  if (result.has_error())
+    libc_errno = result.error;
+  return result.value;
+}
+
+} // namespace LIBC_NAMESPACE
diff --git a/libc/src/math/nanf128.h b/libc/src/math/nanf128.h
new file mode 100644
index 00000000000000..b06d14e2f945e1
--- /dev/null
+++ b/libc/src/math/nanf128.h
@@ -0,0 +1,20 @@
+//===-- Implementation header for nanf128 -----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_MATH_NANF128_H
+#define LLVM_LIBC_SRC_MATH_NANF128_H
+
+#include "src/__support/macros/properties/types.h"
+
+namespace LIBC_NAMESPACE {
+
+float128 nanf128(const char *arg);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_MATH_NANF128_H
diff --git a/libc/test/src/math/smoke/CMakeLists.txt b/libc/test/src/math/smoke/CMakeLists.txt
index 293e65abd44f5a..80ea9d1109a0cf 100644
--- a/libc/test/src/math/smoke/CMakeLists.txt
+++ b/libc/test/src/math/smoke/CMakeLists.txt
@@ -1506,6 +1506,22 @@ add_fp_unittest(
   UNIT_TEST_ONLY
 )
 
+add_fp_unittest(
+  nanf128_test
+  SUITE
+    libc-math-smoke-tests
+  SRCS
+    nanf128_test.cpp
+  DEPENDS
+    libc.include.math
+    libc.include.signal
+    libc.src.math.nanf128
+    libc.src.__support.FPUtil.fp_bits
+  # FIXME: The nan tests currently have death tests, which aren't supported for
+  # hermetic tests.
+  UNIT_TEST_ONLY
+)
+
 add_fp_unittest(
   nextafter_test
   SUITE
diff --git a/libc/test/src/math/smoke/nanf128_test.cpp b/libc/test/src/math/smoke/nanf128_test.cpp
new file mode 100644
index 00000000000000..f90f052ec254d8
--- /dev/null
+++ b/libc/test/src/math/smoke/nanf128_test.cpp
@@ -0,0 +1,64 @@
+//===-- Unittests for nanf128 ---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/FPUtil/FPBits.h"
+#include "src/__support/UInt128.h"
+#include "src/math/nanf128.h"
+#include "test/UnitTest/FPMatcher.h"
+#include "test/UnitTest/Test.h"
+
+class LlvmLibcNanf128Test : public LIBC_NAMESPACE::testing::Test {
+public:
+  using FPBits128 = LIBC_NAMESPACE::fputil::FPBits<float128>;
+  using StorageType = FPBits128::StorageType;
+
+  const UInt128 QUIET_NAN = FPBits128::quiet_nan(
+      LIBC_NAMESPACE::fputil::Sign::POS, 0).uintval();
+
+  const UInt128 ONE = UInt128(1);
+  const UInt128 ZERO = UInt128(0);
+
+  void run_test(const char *input_str, StorageType bits) {
+    float128 result = LIBC_NAMESPACE::nanf128(input_str);
+    auto actual_fp = FPBits128(result);
+    auto expected_fp = FPBits128(bits);
+    EXPECT_EQ(actual_fp.uintval(), expected_fp.uintval());
+  };
+};
+
+TEST_F(LlvmLibcNanf128Test, NCharSeq) {
+  run_test("", QUIET_NAN);
+  run_test("1234", QUIET_NAN | 1234);
+  run_test("0x1234", QUIET_NAN | 0x1234);
+  run_test("2417851639229258349412352", QUIET_NAN | (ONE << 81));
+  run_test("0x200000000000000000000", QUIET_NAN | (ONE << 81));
+  run_test("10384593717069655257060992658440191",
+           QUIET_NAN | (~ZERO & FPBits128::SIG_MASK));
+  run_test("0x1ffffffffffffffffffffffffffff",
+           QUIET_NAN | (~ZERO & FPBits128::SIG_MASK));
+  run_test("10384593717069655257060992658440192", QUIET_NAN);
+  run_test("0x20000000000000000000000000000", QUIET_NAN);
+  run_test("1a", QUIET_NAN);
+  run_test("10000000000000000000000000000000000000000000000000", QUIET_NAN);
+}
+
+TEST_F(LlvmLibcNanf128Test, RandomString) {
+  run_test(" 1234", QUIET_NAN);
+  run_test("-1234", QUIET_NAN);
+  run_test("asd&f", QUIET_NAN);
+  run_test("123 ", QUIET_NAN);
+  run_test("1234567890qwertyuiopasdfghjklzxcvbnmQWERTYUIOPASDFGHJKLZXCVBNM_",
+           QUIET_NAN);
+}
+
+#ifndef LIBC_HAVE_ADDRESS_SANITIZER
+#include <signal.h>
+TEST_F(LlvmLibcNanf128Test, InvalidInput) {
+  EXPECT_DEATH([] { LIBC_NAMESPACE::nanf128(nullptr); }, WITH_SIGNAL(SIGSEGV));
+}
+#endif // LIBC_HAVE_ADDRESS_SANITIZER

Copy link

github-actions bot commented Mar 14, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@lntue lntue changed the title [libc][math] adds nanf128 [libc][math][c23] adds nanf128 Mar 14, 2024
Comment on lines 996 to 1004
// make_unsigned and make_signed type traits
template <size_t Bits, bool Signed, typename T>
struct cpp::make_unsigned<BigInt<Bits, Signed, T>>
: cpp::type_identity<BigInt<Bits, false, T>> {};

template <size_t Bits, bool Signed, typename T>
struct cpp::make_signed<BigInt<Bits, Signed, T>>
: cpp::type_identity<BigInt<Bits, true, T>> {};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were getting away from specializing make_signed and make_unsigned for BigInt class. For the context, see #84299

For getting signed and unsigned types of BigInt, you might refactor https://github.com/llvm/llvm-project/blob/main/libc/src/__support/integer_to_string.h#L153 to add something like: is_integral_or_bigint, make_integral_or_bigint_signed, make_integral_or_bigint_unsigned, together with their _v and _t templates.

Copy link
Contributor Author

@Flandini Flandini Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled the related code out of integer_to_string.h and into UInt.h. I went with make_integral_or_big_int_signed styling on the name to match the other trait in UInt.h.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit verbose but at least it's pretty clear what it does. LGTM.

@lntue lntue merged commit b43965a into llvm:main Mar 15, 2024
5 checks passed
@Flandini Flandini deleted the nanf128 branch April 12, 2024 23:45
michaelrj-google added a commit to michaelrj-google/llvm-project that referenced this pull request Oct 10, 2024
Needed for llvm#110894

The strtointeger code was updated to support some bigint types in llvm#85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.
michaelrj-google added a commit that referenced this pull request Oct 11, 2024
Needed for #110894

The strtointeger code was updated to support some bigint types in #85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
Needed for llvm#110894

The strtointeger code was updated to support some bigint types in llvm#85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
Needed for llvm#110894

The strtointeger code was updated to support some bigint types in llvm#85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants