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] Remove specific nan payload in math functions #79165

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

gchatelet
Copy link
Contributor

No description provided.

@gchatelet gchatelet requested a review from lntue January 23, 2024 16:36
@llvmbot llvmbot added the libc label Jan 23, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 23, 2024

@llvm/pr-subscribers-libc

Author: Guillaume Chatelet (gchatelet)

Changes

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

4 Files Affected:

  • (modified) libc/src/math/generic/asinf.cpp (+1-1)
  • (modified) libc/src/math/generic/sincosf.cpp (+1-3)
  • (modified) libc/test/UnitTest/FPMatcher.h (+2-2)
  • (modified) libc/test/src/__support/FPUtil/fpbits_test.cpp (+6-11)
diff --git a/libc/src/math/generic/asinf.cpp b/libc/src/math/generic/asinf.cpp
index ee8e853063644e..d673bdea6697ad 100644
--- a/libc/src/math/generic/asinf.cpp
+++ b/libc/src/math/generic/asinf.cpp
@@ -109,7 +109,7 @@ LLVM_LIBC_FUNCTION(float, asinf, (float x)) {
       fputil::set_errno_if_required(EDOM);
       fputil::raise_except_if_required(FE_INVALID);
     }
-    return x + FPBits::build_nan(Sign::POS, FPBits::FRACTION_MASK).get_val();
+    return x + FPBits::build_nan().get_val();
   }
 
   // Check for exceptional values
diff --git a/libc/src/math/generic/sincosf.cpp b/libc/src/math/generic/sincosf.cpp
index f12b93a0e69653..f1ad741f9be80e 100644
--- a/libc/src/math/generic/sincosf.cpp
+++ b/libc/src/math/generic/sincosf.cpp
@@ -148,9 +148,7 @@ LLVM_LIBC_FUNCTION(void, sincosf, (float x, float *sinp, float *cosp)) {
       fputil::set_errno_if_required(EDOM);
       fputil::raise_except_if_required(FE_INVALID);
     }
-    *sinp =
-        x +
-        FPBits::build_nan(fputil::Sign::POS, FPBits::FRACTION_MASK).get_val();
+    *sinp = x + FPBits::build_nan().get_val();
     *cosp = *sinp;
     return;
   }
diff --git a/libc/test/UnitTest/FPMatcher.h b/libc/test/UnitTest/FPMatcher.h
index 69da5387d382b5..7c81475ecec3ae 100644
--- a/libc/test/UnitTest/FPMatcher.h
+++ b/libc/test/UnitTest/FPMatcher.h
@@ -69,7 +69,7 @@ template <typename T> struct FPTest : public Test {
   static constexpr T zero = FPBits::zero(Sign::POS).get_val();
   static constexpr T neg_zero = FPBits::zero(Sign::NEG).get_val();
   static constexpr T aNaN = FPBits::build_quiet_nan().get_val();
-  static constexpr T sNaN = FPBits::build_nan(Sign::POS, 1).get_val();
+  static constexpr T sNaN = FPBits::build_nan().get_val();
   static constexpr T inf = FPBits::inf(Sign::POS).get_val();
   static constexpr T neg_inf = FPBits::inf(Sign::NEG).get_val();
   static constexpr T min_normal = FPBits::min_normal().get_val();
@@ -98,7 +98,7 @@ template <typename T> struct FPTest : public Test {
   const T zero = FPBits::zero(Sign::POS).get_val();                            \
   const T neg_zero = FPBits::zero(Sign::NEG).get_val();                        \
   const T aNaN = FPBits::build_quiet_nan().get_val();                          \
-  const T sNaN = FPBits::build_nan(Sign::POS, 1).get_val();                    \
+  const T sNaN = FPBits::build_nan().get_val();                                \
   const T inf = FPBits::inf(Sign::POS).get_val();                              \
   const T neg_inf = FPBits::inf(Sign::NEG).get_val();                          \
   const T min_normal = FPBits::min_normal().get_val();                         \
diff --git a/libc/test/src/__support/FPUtil/fpbits_test.cpp b/libc/test/src/__support/FPUtil/fpbits_test.cpp
index aadcd3fe92b9e5..a4324a071536f8 100644
--- a/libc/test/src/__support/FPUtil/fpbits_test.cpp
+++ b/libc/test/src/__support/FPUtil/fpbits_test.cpp
@@ -236,8 +236,7 @@ TEST(LlvmLibcFPBitsTest, FloatType) {
                "(+Infinity)");
   EXPECT_STREQ(LIBC_NAMESPACE::str(FloatBits::inf(Sign::NEG)).c_str(),
                "(-Infinity)");
-  EXPECT_STREQ(LIBC_NAMESPACE::str(FloatBits::build_nan(Sign::POS, 1)).c_str(),
-               "(NaN)");
+  EXPECT_STREQ(LIBC_NAMESPACE::str(FloatBits::build_nan()).c_str(), "(NaN)");
 
   FloatBits zero(0.0f);
   EXPECT_TRUE(zero.is_pos());
@@ -363,9 +362,8 @@ TEST(LlvmLibcFPBitsTest, X86LongDoubleType) {
                "(+Infinity)");
   EXPECT_STREQ(LIBC_NAMESPACE::str(LongDoubleBits::inf(Sign::NEG)).c_str(),
                "(-Infinity)");
-  EXPECT_STREQ(
-      LIBC_NAMESPACE::str(LongDoubleBits::build_nan(Sign::POS, 1)).c_str(),
-      "(NaN)");
+  EXPECT_STREQ(LIBC_NAMESPACE::str(LongDoubleBits::build_nan()).c_str(),
+               "(NaN)");
 
   LongDoubleBits zero(0.0l);
   EXPECT_TRUE(zero.is_pos());
@@ -444,9 +442,8 @@ TEST(LlvmLibcFPBitsTest, LongDoubleType) {
                "(+Infinity)");
   EXPECT_STREQ(LIBC_NAMESPACE::str(LongDoubleBits::inf(Sign::NEG)).c_str(),
                "(-Infinity)");
-  EXPECT_STREQ(
-      LIBC_NAMESPACE::str(LongDoubleBits::build_nan(Sign::POS, 1)).c_str(),
-      "(NaN)");
+  EXPECT_STREQ(LIBC_NAMESPACE::str(LongDoubleBits::build_nan()).c_str(),
+               "(NaN)");
 
   LongDoubleBits zero(0.0l);
   EXPECT_TRUE(zero.is_pos());
@@ -519,9 +516,7 @@ TEST(LlvmLibcFPBitsTest, Float128Type) {
                "(+Infinity)");
   EXPECT_STREQ(LIBC_NAMESPACE::str(Float128Bits::inf(Sign::NEG)).c_str(),
                "(-Infinity)");
-  EXPECT_STREQ(
-      LIBC_NAMESPACE::str(Float128Bits::build_nan(Sign::POS, 1)).c_str(),
-      "(NaN)");
+  EXPECT_STREQ(LIBC_NAMESPACE::str(Float128Bits::build_nan()).c_str(), "(NaN)");
 
   Float128Bits zero = Float128Bits::zero(Sign::POS);
   EXPECT_TRUE(zero.is_pos());

libc/src/math/generic/asinf.cpp Outdated Show resolved Hide resolved
libc/src/math/generic/sincosf.cpp Outdated Show resolved Hide resolved
@gchatelet
Copy link
Contributor Author

gchatelet commented Jan 29, 2024

@lntue can you PTAL?
If this patch is correct this means we can completely remove FPBits::build_nan

edit: You can review 2fdf373 only, the other one you already reviewed but I had to push force (conflict -> rebase).

@lntue
Copy link
Contributor

lntue commented Jan 29, 2024

@lntue can you PTAL? If this patch is correct this means we can completely remove FPBits::build_nan

edit: You can review 2fdf373 only, the other one you already reviewed but I had to push force (conflict -> rebase).

Yes, most of the time we only need build_quiet_nan, but there might be some rare cases that we need signaling nan. I guess we can remove build_nan, and only use build_quiet_nan + build_signaling_nan to be explicit.

@gchatelet gchatelet merged commit eb56bc2 into llvm:main Jan 29, 2024
4 checks passed
@gchatelet gchatelet deleted the remove_more_nan_payloads branch January 29, 2024 14:28
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.

None yet

3 participants