[llvm] [APFloat] Correct semantics of minimum/maximum for signaling NaN arguments (PR #109976)
Alex Bradbury via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 25 06:15:25 PDT 2024
https://github.com/asb created https://github.com/llvm/llvm-project/pull/109976
The minimum and maximum operations were introduced in
https://reviews.llvm.org/D52764 alongside the intrinsics. The question
of NaN propagation was discussed at the time, but the resulting
semantics don't seem to match what was ultimately agreed in IEEE754-2019
or the description we now have in the LangRef at
<https://llvm.org/docs/LangRef.html#llvm-min-intrinsics-comparation>.
Essentially, the APFloat implementation doesn't quiet a signaling NaN
input when I believe it should in order to match the LangRef and IEEE
spec.
>From 2db60becbf51407b891a8d0c258d7355d02d4099 Mon Sep 17 00:00:00 2001
From: Alex Bradbury <asb at igalia.com>
Date: Wed, 25 Sep 2024 13:53:33 +0100
Subject: [PATCH 1/2] [test][ADT] Pre-commit test for fix in APFloat
minimum/maximum behaviour
Per the langref and IEEE-754 2019 spec, a signaling input should produce
a quiet nan output. This test just captures the current behaviour, prior
to a fix.
---
llvm/unittests/ADT/APFloatTest.cpp | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/llvm/unittests/ADT/APFloatTest.cpp b/llvm/unittests/ADT/APFloatTest.cpp
index 6c49d78e5c8ea9..a976f0a4ee4756 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -607,6 +607,7 @@ TEST(APFloatTest, Minimum) {
APFloat zp(0.0);
APFloat zn(-0.0);
APFloat nan = APFloat::getNaN(APFloat::IEEEdouble());
+ APFloat snan = APFloat::getSNaN(APFloat::IEEEdouble());
EXPECT_EQ(1.0, minimum(f1, f2).convertToDouble());
EXPECT_EQ(1.0, minimum(f2, f1).convertToDouble());
@@ -614,6 +615,10 @@ TEST(APFloatTest, Minimum) {
EXPECT_EQ(-0.0, minimum(zn, zp).convertToDouble());
EXPECT_TRUE(std::isnan(minimum(f1, nan).convertToDouble()));
EXPECT_TRUE(std::isnan(minimum(nan, f1).convertToDouble()));
+ EXPECT_TRUE(maximum(snan, f1).isNaN());
+ // FIXME: the langref and IEEE spec indicate the returned NaN should be
+ // quiet rather than signaling.
+ EXPECT_TRUE(maximum(snan, f1).isSignaling());
}
TEST(APFloatTest, Maximum) {
@@ -622,6 +627,7 @@ TEST(APFloatTest, Maximum) {
APFloat zp(0.0);
APFloat zn(-0.0);
APFloat nan = APFloat::getNaN(APFloat::IEEEdouble());
+ APFloat snan = APFloat::getSNaN(APFloat::IEEEdouble());
EXPECT_EQ(2.0, maximum(f1, f2).convertToDouble());
EXPECT_EQ(2.0, maximum(f2, f1).convertToDouble());
@@ -629,6 +635,10 @@ TEST(APFloatTest, Maximum) {
EXPECT_EQ(0.0, maximum(zn, zp).convertToDouble());
EXPECT_TRUE(std::isnan(maximum(f1, nan).convertToDouble()));
EXPECT_TRUE(std::isnan(maximum(nan, f1).convertToDouble()));
+ EXPECT_TRUE(maximum(snan, f1).isNaN());
+ // FIXME: the langref and IEEE spec indicate the returned NaN should be
+ // quiet rather than signaling.
+ EXPECT_TRUE(maximum(snan, f1).isSignaling());
}
TEST(APFloatTest, MinimumNumber) {
>From fa4e818b6f0f24f9e72d11c06777c9e46e7aa44e Mon Sep 17 00:00:00 2001
From: Alex Bradbury <asb at igalia.com>
Date: Wed, 25 Sep 2024 14:09:01 +0100
Subject: [PATCH 2/2] [APFloat] Correct semantics of minimum/maximum for
signaling NaN arguments
The minimum and maximum operations were introduced in
https://reviews.llvm.org/D52764 alongside the intrinsics. The question
of NaN propagation was discussed at the time, but the resulting
semantics don't seem to match what was ultimately agreed in IEEE754-2019
or the description we now have in the LangRef at
<https://llvm.org/docs/LangRef.html#llvm-min-intrinsics-comparation>.
Essentially, the APFloat implementation doesn't quiet a signaling NaN
input when I believe it should in order to match the LangRef and IEEE
spec.
---
llvm/include/llvm/ADT/APFloat.h | 14 ++++++++------
llvm/unittests/ADT/APFloatTest.cpp | 8 ++------
2 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index 9cc8369a0bf52b..acb3b2e2103009 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/llvm/include/llvm/ADT/APFloat.h
@@ -1483,13 +1483,14 @@ inline APFloat maxnum(const APFloat &A, const APFloat &B) {
}
/// Implements IEEE 754-2019 minimum semantics. Returns the smaller of 2
-/// arguments, propagating NaNs and treating -0 as less than +0.
+/// arguments, returning a quiet NaN if an argument is a NaN and treating -0
+/// as less than +0.
LLVM_READONLY
inline APFloat minimum(const APFloat &A, const APFloat &B) {
if (A.isNaN())
- return A;
+ return A.makeQuiet();
if (B.isNaN())
- return B;
+ return B.makeQuiet();
if (A.isZero() && B.isZero() && (A.isNegative() != B.isNegative()))
return A.isNegative() ? A : B;
return B < A ? B : A;
@@ -1509,13 +1510,14 @@ inline APFloat minimumnum(const APFloat &A, const APFloat &B) {
}
/// Implements IEEE 754-2019 maximum semantics. Returns the larger of 2
-/// arguments, propagating NaNs and treating -0 as less than +0.
+/// arguments, returning a quiet NaN if an argument is a NaN and treating -0
+/// as less than +0.
LLVM_READONLY
inline APFloat maximum(const APFloat &A, const APFloat &B) {
if (A.isNaN())
- return A;
+ return A.makeQuiet();
if (B.isNaN())
- return B;
+ return B.makeQuiet();
if (A.isZero() && B.isZero() && (A.isNegative() != B.isNegative()))
return A.isNegative() ? B : A;
return A < B ? B : A;
diff --git a/llvm/unittests/ADT/APFloatTest.cpp b/llvm/unittests/ADT/APFloatTest.cpp
index a976f0a4ee4756..29149ffadfb6a7 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -616,9 +616,7 @@ TEST(APFloatTest, Minimum) {
EXPECT_TRUE(std::isnan(minimum(f1, nan).convertToDouble()));
EXPECT_TRUE(std::isnan(minimum(nan, f1).convertToDouble()));
EXPECT_TRUE(maximum(snan, f1).isNaN());
- // FIXME: the langref and IEEE spec indicate the returned NaN should be
- // quiet rather than signaling.
- EXPECT_TRUE(maximum(snan, f1).isSignaling());
+ EXPECT_FALSE(maximum(snan, f1).isSignaling());
}
TEST(APFloatTest, Maximum) {
@@ -636,9 +634,7 @@ TEST(APFloatTest, Maximum) {
EXPECT_TRUE(std::isnan(maximum(f1, nan).convertToDouble()));
EXPECT_TRUE(std::isnan(maximum(nan, f1).convertToDouble()));
EXPECT_TRUE(maximum(snan, f1).isNaN());
- // FIXME: the langref and IEEE spec indicate the returned NaN should be
- // quiet rather than signaling.
- EXPECT_TRUE(maximum(snan, f1).isSignaling());
+ EXPECT_FALSE(maximum(snan, f1).isSignaling());
}
TEST(APFloatTest, MinimumNumber) {
More information about the llvm-commits
mailing list