Skip to content

[APFloat] Correct semantics of minimum/maximum for signaling NaN arguments #109976

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 1, 2024
Merged

[APFloat] Correct semantics of minimum/maximum for signaling NaN arguments #109976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2db60be
[test][ADT] Pre-commit test for fix in APFloat minimum/maximum behaviour
asb Sep 25, 2024
fa4e818
[APFloat] Correct semantics of minimum/maximum for signaling NaN argu…
asb Sep 25, 2024
6bd8a30
Test snan in other position as well
asb Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions llvm/include/llvm/ADT/APFloat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
10 changes: 10 additions & 0 deletions llvm/unittests/ADT/APFloatTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,13 +607,18 @@ 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());
EXPECT_EQ(-0.0, minimum(zp, zn).convertToDouble());
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_TRUE(maximum(f1, snan).isNaN());
EXPECT_FALSE(maximum(snan, f1).isSignaling());
Copy link
Contributor

@arsenm arsenm Sep 25, 2024

Choose a reason for hiding this comment

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

Maybe could use a test that the payload and signbit are preserved, but it's not important

Copy link
Contributor Author

@asb asb Sep 25, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@arsenm arsenm Sep 25, 2024

Choose a reason for hiding this comment

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

It is not required, but suggested you should try to preserve the payload of one of the input operands for nan producing operations

EXPECT_FALSE(maximum(f1, snan).isSignaling());
}
Copy link
Contributor

@arsenm arsenm Sep 25, 2024

Choose a reason for hiding this comment

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

Also test with snan in both positions


TEST(APFloatTest, Maximum) {
Expand All @@ -622,13 +627,18 @@ 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());
EXPECT_EQ(0.0, maximum(zp, zn).convertToDouble());
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_TRUE(maximum(f1, snan).isNaN());
EXPECT_FALSE(maximum(snan, f1).isSignaling());
EXPECT_FALSE(maximum(f1, snan).isSignaling());
}

TEST(APFloatTest, MinimumNumber) {
Expand Down
Loading