[libc][math] Update getpayload and fmul with NaN inputs. #99812

lntue · 2024-07-21T15:56:55Z

No description provided.

llvmbot · 2024-07-21T15:57:27Z

@llvm/pr-subscribers-libc

Author: None (lntue)

Changes

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

4 Files Affected:

(modified) libc/src/__support/FPUtil/BasicOperations.h (+5-2)
(modified) libc/src/__support/FPUtil/CMakeLists.txt (+29-28)
(modified) libc/src/__support/FPUtil/generic/mul.h (+10-10)
(modified) libc/test/src/math/smoke/MulTest.h (+2-17)

diff --git a/libc/src/__support/FPUtil/BasicOperations.h b/libc/src/__support/FPUtil/BasicOperations.h
index a963a92bfb074..40f37b281b02c 100644
--- a/libc/src/__support/FPUtil/BasicOperations.h
+++ b/libc/src/__support/FPUtil/BasicOperations.h
@@ -11,8 +11,8 @@
 
 #include "FEnvImpl.h"
 #include "FPBits.h"
+#include "dyadic_float.h"
 
-#include "FEnvImpl.h"
 #include "src/__support/CPP/type_traits.h"
 #include "src/__support/common.h"
 #include "src/__support/macros/config.h"
@@ -274,7 +274,10 @@ LIBC_INLINE cpp::enable_if_t<cpp::is_floating_point_v<T>, T> getpayload(T x) {
   if (!x_bits.is_nan())
     return T(-1.0);
 
-  return T(x_bits.uintval() & (FPBits::FRACTION_MASK >> 1));
+  DyadicFloat<FPBits::STORAGE_LEN> payload(
+      Sign::POS, 0, x_bits.uintval() & (FPBits::FRACTION_MASK >> 1));
+
+  return static_cast<T>(payload);
 }
 
 template <bool IsSignaling, typename T>
diff --git a/libc/src/__support/FPUtil/CMakeLists.txt b/libc/src/__support/FPUtil/CMakeLists.txt
index 793d3a121c742..8804f3a4d5e23 100644
--- a/libc/src/__support/FPUtil/CMakeLists.txt
+++ b/libc/src/__support/FPUtil/CMakeLists.txt
@@ -75,19 +75,6 @@ add_header_library(
     libc.src.__support.common
 )
 
-add_header_library(
-  basic_operations
-  HDRS
-    BasicOperations.h
-  DEPENDS
-    .fp_bits
-    .fenv_impl
-    libc.src.__support.CPP.type_traits
-    libc.src.__support.uint128
-    libc.src.__support.common
-    libc.src.__support.macros.optimization
-)
-
 add_header_library(
   division_and_remainder_operations
   HDRS
@@ -113,21 +100,6 @@ add_header_library(
 )
 
 
-add_header_library(
-  hypot
-  HDRS
-    Hypot.h
-  DEPENDS
-    .basic_operations
-    .fenv_impl
-    .fp_bits
-    .rounding_mode
-    libc.src.__support.common
-    libc.src.__support.CPP.bit
-    libc.src.__support.CPP.type_traits
-    libc.src.__support.uint128
-)
-
 add_header_library(
   sqrt
   HDRS
@@ -208,6 +180,35 @@ add_header_library(
     libc.src.__support.macros.optimization
 )
 
+add_header_library(
+  basic_operations
+  HDRS
+    BasicOperations.h
+  DEPENDS
+    .dyadic_float
+    .fp_bits
+    .fenv_impl
+    libc.src.__support.CPP.type_traits
+    libc.src.__support.uint128
+    libc.src.__support.common
+    libc.src.__support.macros.optimization
+)
+
+add_header_library(
+  hypot
+  HDRS
+    Hypot.h
+  DEPENDS
+    .basic_operations
+    .fenv_impl
+    .fp_bits
+    .rounding_mode
+    libc.src.__support.common
+    libc.src.__support.CPP.bit
+    libc.src.__support.CPP.type_traits
+    libc.src.__support.uint128
+)
+
 add_header_library(
   manipulation_functions
   HDRS
diff --git a/libc/src/__support/FPUtil/generic/mul.h b/libc/src/__support/FPUtil/generic/mul.h
index 02fc69c6cb1ba..61be3719e123a 100644
--- a/libc/src/__support/FPUtil/generic/mul.h
+++ b/libc/src/__support/FPUtil/generic/mul.h
@@ -50,19 +50,19 @@ mul(InType x, InType y) {
         raise_except_if_required(FE_INVALID);
 
       if (x_bits.is_quiet_nan()) {
-        InStorageType x_payload = static_cast<InStorageType>(getpayload(x));
-        if ((x_payload & ~(OutFPBits::FRACTION_MASK >> 1)) == 0)
-          return OutFPBits::quiet_nan(x_bits.sign(),
-                                      static_cast<OutStorageType>(x_payload))
-              .get_val();
+        InStorageType x_payload = x_bits.get_mantissa();
+        x_payload >>= (InFPBits::FRACTION_LEN - OutFPBits::FRACTION_LEN);
+        return OutFPBits::quiet_nan(x_bits.sign(),
+                                    static_cast<OutStorageType>(x_payload))
+            .get_val();
       }
 
       if (y_bits.is_quiet_nan()) {
-        InStorageType y_payload = static_cast<InStorageType>(getpayload(y));
-        if ((y_payload & ~(OutFPBits::FRACTION_MASK >> 1)) == 0)
-          return OutFPBits::quiet_nan(y_bits.sign(),
-                                      static_cast<OutStorageType>(y_payload))
-              .get_val();
+        InStorageType y_payload = y_bits.get_mantissa();
+        y_payload >>= (InFPBits::FRACTION_LEN - OutFPBits::FRACTION_LEN);
+        return OutFPBits::quiet_nan(y_bits.sign(),
+                                    static_cast<OutStorageType>(y_payload))
+            .get_val();
       }
 
       return OutFPBits::quiet_nan().get_val();
diff --git a/libc/test/src/math/smoke/MulTest.h b/libc/test/src/math/smoke/MulTest.h
index e2298eaeeb216..0c847e39687b7 100644
--- a/libc/test/src/math/smoke/MulTest.h
+++ b/libc/test/src/math/smoke/MulTest.h
@@ -38,23 +38,8 @@ class MulTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
     EXPECT_FP_IS_NAN_WITH_EXCEPTION(func(sNaN, sNaN), FE_INVALID);
 
     InType qnan_42 = InFPBits::quiet_nan(Sign::POS, 0x42).get_val();
-    EXPECT_FP_EQ(InType(0x42.0p+0),
-                 LIBC_NAMESPACE::fputil::getpayload(func(qnan_42, zero)));
-    EXPECT_FP_EQ(InType(0x42.0p+0),
-                 LIBC_NAMESPACE::fputil::getpayload(func(zero, qnan_42)));
-
-    if constexpr (sizeof(OutType) < sizeof(InType)) {
-      InStorageType max_payload = InFPBits::FRACTION_MASK >> 1;
-      InType qnan_max = InFPBits::quiet_nan(Sign::POS, max_payload).get_val();
-      EXPECT_FP_EQ(zero,
-                   LIBC_NAMESPACE::fputil::getpayload(func(qnan_max, zero)));
-      EXPECT_FP_EQ(zero,
-                   LIBC_NAMESPACE::fputil::getpayload(func(zero, qnan_max)));
-      EXPECT_FP_EQ(InType(0x42.0p+0),
-                   LIBC_NAMESPACE::fputil::getpayload(func(qnan_max, qnan_42)));
-      EXPECT_FP_EQ(InType(0x42.0p+0),
-                   LIBC_NAMESPACE::fputil::getpayload(func(qnan_42, qnan_max)));
-    }
+    EXPECT_FP_IS_NAN(func(qnan_42, zero));
+    EXPECT_FP_IS_NAN(func(zero, qnan_42));
 
     EXPECT_FP_EQ(inf, func(inf, InType(1.0)));
     EXPECT_FP_EQ(neg_inf, func(neg_inf, InType(1.0)));

overmighty · 2024-07-21T17:46:14Z

libc/src/__support/FPUtil/BasicOperations.h

-  return T(x_bits.uintval() & (FPBits::FRACTION_MASK >> 1));
+  DyadicFloat<FPBits::STORAGE_LEN> payload(
+      Sign::POS, 0, x_bits.uintval() & (FPBits::FRACTION_MASK >> 1));
+
+  return static_cast<T>(payload);


DyadicFloat::operator T()/DyadicFloat::as() is much more complex than the built-in conversion from integer to floating-point. We should probably use the built-in conversion when we can, so just not when FPBits::StorageType is a BigInt apparently.

libc/src/__support/FPUtil/CMakeLists.txt

overmighty · 2024-07-21T18:13:48Z

libc/src/__support/FPUtil/generic/mul.h

+        InStorageType x_payload = x_bits.get_mantissa();
+        x_payload >>= (InFPBits::FRACTION_LEN - OutFPBits::FRACTION_LEN);
+        return OutFPBits::quiet_nan(x_bits.sign(),
+                                    static_cast<OutStorageType>(x_payload))
+            .get_val();


IEEE Std 754-2019, section 6.2.3 "NaN propagation":

An operation that propagates a NaN operand to its result and has a single NaN as an input should produce a
NaN with the payload of the input NaN if representable in the destination format.

[...]

Conversion of a quiet NaN to a floating-point format of the same or a different radix that does not allow
the payload to be preserved shall return a quiet NaN that should provide some language-defined diagnostic
information.

But I'm not sure if conversion covers rounding the result to a narrower type in this case, and if the second paragraph is complementary to the first one (an operation that propagates a NaN operand should return a quiet NaN with language-defined diagnostic information if the payload of the input NaN is not representable in the destination format) or if it's just a different case (an operation that propagates a NaN operand can return a NaN with whatever payload if the payload of the input NaN is not representable in the destination format).

Maybe conversion just means conversion operators/functions. glibc seems to just truncate the lower bits of the mantissa: https://godbolt.org/z/aP7Yhccje.

Most of the nan-propagations, especially binary operations, seem to be best-effort, and a bit underspecified in both IEEE 754 and C standards.

overmighty · 2024-07-21T18:15:42Z

libc/src/__support/FPUtil/generic/mul.h

-                                      static_cast<OutStorageType>(x_payload))
-              .get_val();
+        InStorageType x_payload = x_bits.get_mantissa();
+        x_payload >>= (InFPBits::FRACTION_LEN - OutFPBits::FRACTION_LEN);


Parentheses should not be necessary since this is >>=, not >>.

overmighty · 2024-07-21T18:16:31Z

libc/test/src/math/smoke/MulTest.h

+    EXPECT_FP_IS_NAN(func(qnan_42, zero));
+    EXPECT_FP_IS_NAN(func(zero, qnan_42));


Are you sure you don't want to check the payload?

Yes, I don't think we need to be that strict on NaN propagation when it is underspecified in the standards, as long as the NaN semantic is preserved.

mikhailramalho

This patch fixes the compilation and the test failures in rv32. Thanks for the patch!

mikhailramalho · 2024-07-21T21:11:59Z

Could you also add the following to this PR?

diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index 54a382eccb54..f1be6a3f8606 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -377,6 +377,7 @@ set(TARGET_LIBM_ENTRYPOINTS
     libc.src.math.cosf
     libc.src.math.coshf
     libc.src.math.cospif
+    libc.src.math.dmull
     libc.src.math.erff
     libc.src.math.exp
     libc.src.math.exp10
@@ -433,6 +434,7 @@ set(TARGET_LIBM_ENTRYPOINTS
     libc.src.math.libc
     fmodf.src.math.fmodl
     libc.src.math.fmul
+    libc.src.math.fmull
     libc.src.math.frexp
     libc.src.math.frexpf
     libc.src.math.frexpl

mikhailramalho

LGTM

…s. (#99812) Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251352

lntue requested review from mikhailramalho and overmighty July 21, 2024 15:56

llvmbot added the libc label Jul 21, 2024

overmighty reviewed Jul 21, 2024

View reviewed changes

mikhailramalho approved these changes Jul 21, 2024

View reviewed changes

lntue added 4 commits July 21, 2024 22:09

[libc][math] Update getpayload and fmul with NaN inputs.

8ef05d3

Address comments and fix the same issue for add_sub, div, and FMA.

0570965

Fix add + sub tests and fma.

dbd831d

Add missing mul entrypoints for riscv.

1e4d04e

lntue force-pushed the payload branch from badf53b to 1e4d04e Compare July 21, 2024 22:10

Update DivTest.h

fb6525d

lntue requested review from mikhailramalho and overmighty July 21, 2024 23:15

mikhailramalho approved these changes Jul 21, 2024

View reviewed changes

lntue merged commit 74a1ca5 into llvm:main Jul 21, 2024
6 checks passed

lntue deleted the payload branch July 21, 2024 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc][math] Update getpayload and fmul with NaN inputs. #99812

[libc][math] Update getpayload and fmul with NaN inputs. #99812

Uh oh!

lntue commented Jul 21, 2024

Uh oh!

llvmbot commented Jul 21, 2024

Uh oh!

overmighty Jul 21, 2024

Uh oh!

lntue Jul 21, 2024

Uh oh!

Uh oh!

overmighty Jul 21, 2024

Uh oh!

lntue Jul 21, 2024

Uh oh!

overmighty Jul 21, 2024

Uh oh!

overmighty Jul 21, 2024

Uh oh!

lntue Jul 21, 2024

Uh oh!

mikhailramalho left a comment

Uh oh!

mikhailramalho commented Jul 21, 2024

Uh oh!

mikhailramalho left a comment

Uh oh!

Uh oh!

Uh oh!

		EXPECT_FP_IS_NAN(func(qnan_42, zero));
		EXPECT_FP_IS_NAN(func(zero, qnan_42));

[libc][math] Update getpayload and fmul with NaN inputs. #99812

[libc][math] Update getpayload and fmul with NaN inputs. #99812

Uh oh!

Conversation

lntue commented Jul 21, 2024

Uh oh!

llvmbot commented Jul 21, 2024

Uh oh!

overmighty Jul 21, 2024

Choose a reason for hiding this comment

Uh oh!

lntue Jul 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

overmighty Jul 21, 2024

Choose a reason for hiding this comment

Uh oh!

lntue Jul 21, 2024

Choose a reason for hiding this comment

Uh oh!

overmighty Jul 21, 2024

Choose a reason for hiding this comment

Uh oh!

overmighty Jul 21, 2024

Choose a reason for hiding this comment

Uh oh!

lntue Jul 21, 2024

Choose a reason for hiding this comment

Uh oh!

mikhailramalho left a comment

Choose a reason for hiding this comment

Uh oh!

mikhailramalho commented Jul 21, 2024

Uh oh!

mikhailramalho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!