[APFloat] Correct semantics of minimum/maximum for signaling NaN arguments #109976
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
…ments 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.
asb
requested review from
arsenm,
wzssyqa,
preames,
tlively,
topperc and
andykaylor
|
@llvm/pr-subscribers-llvm-adt Author: Alex Bradbury (asb) ChangesThe minimum and maximum operations were introduced in Essentially, the APFloat implementation doesn't quiet a signaling NaN Full diff: https://github.com/llvm/llvm-project/pull/109976.diff 2 Files Affected:
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 6c49d78e5c8ea9..29149ffadfb6a7 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,8 @@ 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());
+ EXPECT_FALSE(maximum(snan, f1).isSignaling());
}
TEST(APFloatTest, Maximum) {
@@ -622,6 +625,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 +633,8 @@ 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());
+ EXPECT_FALSE(maximum(snan, f1).isSignaling());
}
TEST(APFloatTest, MinimumNumber) {
|
arsenm
approved these changes
Sep 25, 2024
| EXPECT_TRUE(std::isnan(minimum(f1, nan).convertToDouble())); | ||
| EXPECT_TRUE(std::isnan(minimum(nan, f1).convertToDouble())); | ||
| EXPECT_TRUE(maximum(snan, f1).isNaN()); | ||
| EXPECT_FALSE(maximum(snan, f1).isSignaling()); |
There was a problem hiding this comment.
Maybe could use a test that the payload and signbit are preserved, but it's not important
There was a problem hiding this comment.
I'm not sure to what degree I should try to closely match the current APFloat implementation vs what the IEEE spec requires. To my cursory reading it looks like it only specifies "a" quiet NaN - but perhaps there's more detail on preserving payload and signbit for NaN operands elsewhere?
There was a problem hiding this comment.
It is not required, but suggested you should try to preserve the payload of one of the input operands for nan producing operations
arsenm
reviewed
Sep 25, 2024
| EXPECT_TRUE(std::isnan(minimum(nan, f1).convertToDouble())); | ||
| EXPECT_TRUE(maximum(snan, f1).isNaN()); | ||
| EXPECT_FALSE(maximum(snan, f1).isSignaling()); | ||
| } |
There was a problem hiding this comment.
Also test with snan in both positions
arsenm
approved these changes
Oct 1, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/6426 Here is the relevant piece of the build log for the reference |
Sterling-Augustine
pushed a commit
to Sterling-Augustine/llvm-project
that referenced
this pull request
Oct 3, 2024
…ments (llvm#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 it should in order to match the LangRef and IEEE spec.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.