[APFloat] Fix `IEEEFloat::addOrSubtractSignificand` and `IEEEFloat::normalize` #98721

beetrees · 2024-07-13T07:22:28Z

Before this PR, addOrSubtractSignificand presumed that the loss came from the side being subtracted, and didn't handle the case where lhs == rhs and there was loss. This can occur during FMA. This PR fixes the situation by correctly determining where the loss came from and handling it appropriately.

Additionally, normalize failed to adjust the exponent when the significand is zero but lost_fraction != lfExactlyZero. This meant that the test case from #63895 was rounded incorrectly as the loss wasn't adjusted to account for the exponent being below the minimum exponent. This PR fixes this by only skipping the exponent adjustment if the significand is zero and there was no lost fraction.

(Note to reviewer: I don't have commit access)

llvmbot · 2024-07-13T07:22:58Z

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-llvm-adt

Author: None (beetrees)

Changes

Fixes #63895

Before this PR, addOrSubtractSignificand presumed that the loss came from the side being subtracted, and didn't handle the case where lhs == rhs and there was loss. This can occur during FMA. This PR fixes the situation by correctly determining where the loss came from and handling it appropriately.

Additionally, normalize failed to adjust the exponent when the significand is zero but lost_fraction != lfExactlyZero. This meant that the test case from #63895 was rounded incorrectly as the loss wasn't adjusted to account for the exponent being below the minimum exponent. This PR fixes this by only skipping the exponent adjustment if the significand is zero and there was no lost fraction.

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

2 Files Affected:

(modified) llvm/lib/Support/APFloat.cpp (+36-15)
(modified) llvm/unittests/ADT/APFloatTest.cpp (+10)

diff --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index 3664de71d06df..46fe1af79a084 100644
--- a/llvm/lib/Support/APFloat.cpp
+++ b/llvm/lib/Support/APFloat.cpp
@@ -1607,7 +1607,8 @@ IEEEFloat::opStatus IEEEFloat::normalize(roundingMode rounding_mode,
   /* Before rounding normalize the exponent of fcNormal numbers.  */
   omsb = significandMSB() + 1;
 
-  if (omsb) {
+  // Only skip this `if` if the value is exactly zero.
+  if (omsb || lost_fraction != lfExactlyZero) {
     /* OMSB is numbered from 1.  We want to place it in the integer
        bit numbered PRECISION if possible, with a compensating change in
        the exponent.  */
@@ -1782,7 +1783,7 @@ IEEEFloat::opStatus IEEEFloat::addOrSubtractSpecials(const IEEEFloat &rhs,
 /* Add or subtract two normal numbers.  */
 lostFraction IEEEFloat::addOrSubtractSignificand(const IEEEFloat &rhs,
                                                  bool subtract) {
-  integerPart carry;
+  integerPart carry = 0;
   lostFraction lost_fraction;
   int bits;
 
@@ -1796,11 +1797,13 @@ lostFraction IEEEFloat::addOrSubtractSignificand(const IEEEFloat &rhs,
   /* Subtraction is more subtle than one might naively expect.  */
   if (subtract) {
     IEEEFloat temp_rhs(rhs);
+    auto lost_fraction_is_from_rhs = false;
 
     if (bits == 0)
       lost_fraction = lfExactlyZero;
     else if (bits > 0) {
       lost_fraction = temp_rhs.shiftSignificandRight(bits - 1);
+      lost_fraction_is_from_rhs = true;
       shiftSignificandLeft(1);
     } else {
       lost_fraction = shiftSignificandRight(-bits - 1);
@@ -1808,23 +1811,41 @@ lostFraction IEEEFloat::addOrSubtractSignificand(const IEEEFloat &rhs,
     }
 
     // Should we reverse the subtraction.
-    if (compareAbsoluteValue(temp_rhs) == cmpLessThan) {
-      carry = temp_rhs.subtractSignificand
-        (*this, lost_fraction != lfExactlyZero);
+    auto cmp_result = compareAbsoluteValue(temp_rhs);
+    if (cmp_result == cmpLessThan) {
+      auto borrow = false;
+      if (lost_fraction != lfExactlyZero && !lost_fraction_is_from_rhs) {
+        // The lost fraction is being subtracted, borrow from the significand
+        // and invert `lost_fraction`.
+        borrow = true;
+        if (lost_fraction == lfLessThanHalf)
+          lost_fraction = lfMoreThanHalf;
+        else if (lost_fraction == lfMoreThanHalf)
+          lost_fraction = lfLessThanHalf;
+      }
+      carry = temp_rhs.subtractSignificand(*this, borrow);
       copySignificand(temp_rhs);
       sign = !sign;
-    } else {
-      carry = subtractSignificand
-        (temp_rhs, lost_fraction != lfExactlyZero);
+    } else if (cmp_result == cmpGreaterThan) {
+      auto borrow = false;
+      if (lost_fraction != lfExactlyZero && lost_fraction_is_from_rhs) {
+        // The lost fraction is being subtracted, borrow from the significand
+        // and invert `lost_fraction`.
+        borrow = true;
+        if (lost_fraction == lfLessThanHalf)
+          lost_fraction = lfMoreThanHalf;
+        else if (lost_fraction == lfMoreThanHalf)
+          lost_fraction = lfLessThanHalf;
+      }
+      carry = subtractSignificand(temp_rhs, borrow);
+    } else { // cmpEqual
+      zeroSignificand();
+      if (lost_fraction != lfExactlyZero && lost_fraction_is_from_rhs) {
+        // rhs is slightly larger due to the lost fraction, flip the sign.
+        sign = !sign;
+      }
     }
 
-    /* Invert the lost fraction - it was on the RHS and
-       subtracted.  */
-    if (lost_fraction == lfLessThanHalf)
-      lost_fraction = lfMoreThanHalf;
-    else if (lost_fraction == lfMoreThanHalf)
-      lost_fraction = lfLessThanHalf;
-
     /* The code above is intended to ensure that no borrow is
        necessary.  */
     assert(!carry);
diff --git a/llvm/unittests/ADT/APFloatTest.cpp b/llvm/unittests/ADT/APFloatTest.cpp
index 86a25f4394e19..d6c488e75d335 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -560,6 +560,16 @@ TEST(APFloatTest, FMA) {
     EXPECT_EQ(-8.85242279E-41f, f1.convertToFloat());
   }
 
+  // Regression test for failing the `assert(!carry)` in
+  // `addOrSubtractSignificand`.
+  {
+    APFloat f1(-1.4728589E-38f);
+    APFloat f2(3.7105144E-6f);
+    APFloat f3(5.5E-44f);
+    f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
+    EXPECT_EQ(-0.0f, f1.convertToFloat());
+  }
+
   // Test using only a single instance of APFloat.
   {
     APFloat F(1.5);

llvm/lib/Support/APFloat.cpp

tschuett · 2024-07-13T10:33:57Z

The title claims to fix addOrSubtractSignificand and normalize, but there seems to be only a test for the former.

beetrees · 2024-07-13T15:38:54Z

The test will fail if both fixes are not applied. I've updated the test comment to mention that.

llvm/unittests/ADT/APFloatTest.cpp

tschuett · 2024-07-20T15:58:15Z

ÌEEEFloat is currently final. Could make it not final with a comment: "not final for testing".
Make addOrSubtractSignificand protected. Then you can inherit from IEEEFloat for testing.

llvm/lib/Support/APFloat.cpp

beetrees · 2024-07-25T20:39:42Z

ÌEEEFloat is currently final. Could make it not final with a comment: "not final for testing". Make addOrSubtractSignificand protected. Then you can inherit from IEEEFloat for testing.

Done. I also had to make the fields of IEEEFloat protected.

arsenm · 2024-07-26T10:56:05Z

llvm/include/llvm/ADT/APFloat.h

@@ -306,7 +306,8 @@ struct APFloatBase {

 namespace detail {

-class IEEEFloat final : public APFloatBase {
+// Not final for testing purposes only.


I'd prefer to leave it as final

addOrSubtractSignificand went from private to protected. It is the only non-fishy way to get access for testing.

arsenm · 2024-07-26T10:57:29Z

llvm/unittests/ADT/APFloatTest.cpp

+      // `addOrSubtractSignificand` only uses the sign, exponent and significand
+      this->sign = sign;
+      this->exponent = exponent;
+      this->significand.part = significand;


I don't see why this justifies adding a subclass. You can just construct the appropriate value?

The fields and addOrSubtractSignificand are protected so the only way to access them is via a subclass. Unfortunately just constructing a value via the public constructors is not sufficient as non-normalized significands are required to hit all the edge cases in addOrSubtractSignificand.

What's the problem with adding public accessors for these? Or adding a test class as a friend?

arsenm

lgtm but I think there are better options than making this protected. Can the test function just be a friend, adding public accessors, or adding another base class for the tester subclass

arsenm · 2024-08-01T16:02:41Z

llvm/unittests/ADT/APFloatTest.cpp

+      // `addOrSubtractSignificand` only uses the sign, exponent and significand
+      this->sign = sign;
+      this->exponent = exponent;
+      this->significand.part = significand;


What's the problem with adding public accessors for these? Or adding a test class as a friend?

beetrees · 2024-08-02T09:22:44Z

Either public accessors specifically for testing or a friend test class would also work. Is there any preference to which solution is best to use?

RalfJung · 2024-11-20T20:44:43Z

@beetrees @arsenm what would it take to make progress on this PR? Would be nice to get this fixed :)

…ormalize`

beetrees · 2024-11-25T18:57:28Z

I've rebased to fix the merge conflict and added the test cases suggested in #104984. I've also changed the AddOrSubtractSignificand unit test to use a friend class as this can be done with only a single line needing to be added to APFloat.h.

llvmbot added llvm:support llvm:adt labels Jul 13, 2024

tschuett reviewed Jul 13, 2024

View reviewed changes

llvm/lib/Support/APFloat.cpp Outdated Show resolved Hide resolved

tschuett reviewed Jul 13, 2024

View reviewed changes

llvm/lib/Support/APFloat.cpp Outdated Show resolved Hide resolved

beetrees mentioned this pull request Jul 13, 2024

Wrong results in some FMA cases rust-lang/rustc_apfloat#11

Open

beetrees force-pushed the fix-apfloat-fma branch 2 times, most recently from cf8c805 to ecd06e2 Compare July 13, 2024 07:51

tschuett requested a review from arsenm July 13, 2024 10:17

tschuett added the floating-point Floating-point math label Jul 13, 2024

beetrees force-pushed the fix-apfloat-fma branch from ecd06e2 to 2d0f63f Compare July 13, 2024 15:38

nikic requested a review from efriedma-quic July 13, 2024 17:04

arsenm reviewed Jul 15, 2024

View reviewed changes

llvm/unittests/ADT/APFloatTest.cpp Show resolved Hide resolved

beetrees force-pushed the fix-apfloat-fma branch from 2d0f63f to 3f36539 Compare July 20, 2024 13:51

arsenm reviewed Jul 22, 2024

View reviewed changes

llvm/lib/Support/APFloat.cpp Outdated Show resolved Hide resolved

beetrees force-pushed the fix-apfloat-fma branch from 3f36539 to 9b17751 Compare July 25, 2024 20:38

arsenm reviewed Jul 26, 2024

View reviewed changes

arsenm approved these changes Aug 1, 2024

View reviewed changes

beetrees mentioned this pull request Aug 21, 2024

APFloat's fusedMultiplyAdd gives an incorrectly rounded result for IEEEfloat #104984

Open

beetrees force-pushed the fix-apfloat-fma branch from 9b17751 to eabd142 Compare November 25, 2024 17:46

[APFloat] Fix IEEEFloat::addOrSubtractSignificand and `IEEEFloat::n…

0dec30d

…ormalize`

beetrees force-pushed the fix-apfloat-fma branch from eabd142 to 0dec30d Compare November 25, 2024 18:54

beetrees requested a review from tschuett November 25, 2024 18:57

beetrees requested a review from arsenm November 25, 2024 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APFloat] Fix `IEEEFloat::addOrSubtractSignificand` and `IEEEFloat::normalize` #98721

[APFloat] Fix `IEEEFloat::addOrSubtractSignificand` and `IEEEFloat::normalize` #98721

beetrees commented Jul 13, 2024 •

edited

Loading

llvmbot commented Jul 13, 2024 •

edited

Loading

tschuett commented Jul 13, 2024

beetrees commented Jul 13, 2024 •

edited

Loading

tschuett commented Jul 20, 2024

beetrees commented Jul 25, 2024

arsenm Jul 26, 2024

tschuett Jul 26, 2024

arsenm Jul 26, 2024

beetrees Jul 27, 2024

arsenm Aug 1, 2024

arsenm left a comment

arsenm Aug 1, 2024

beetrees commented Aug 2, 2024

RalfJung commented Nov 20, 2024

beetrees commented Nov 25, 2024

[APFloat] Fix IEEEFloat::addOrSubtractSignificand and IEEEFloat::normalize #98721

Are you sure you want to change the base?

[APFloat] Fix IEEEFloat::addOrSubtractSignificand and IEEEFloat::normalize #98721

Conversation

beetrees commented Jul 13, 2024 • edited Loading

llvmbot commented Jul 13, 2024 • edited Loading

tschuett commented Jul 13, 2024

beetrees commented Jul 13, 2024 • edited Loading

tschuett commented Jul 20, 2024

beetrees commented Jul 25, 2024

arsenm Jul 26, 2024

Choose a reason for hiding this comment

tschuett Jul 26, 2024

Choose a reason for hiding this comment

arsenm Jul 26, 2024

Choose a reason for hiding this comment

beetrees Jul 27, 2024

Choose a reason for hiding this comment

arsenm Aug 1, 2024

Choose a reason for hiding this comment

arsenm left a comment

Choose a reason for hiding this comment

arsenm Aug 1, 2024

Choose a reason for hiding this comment

beetrees commented Aug 2, 2024

RalfJung commented Nov 20, 2024

beetrees commented Nov 25, 2024

[APFloat] Fix `IEEEFloat::addOrSubtractSignificand` and `IEEEFloat::normalize` #98721

[APFloat] Fix `IEEEFloat::addOrSubtractSignificand` and `IEEEFloat::normalize` #98721

beetrees commented Jul 13, 2024 •

edited

Loading

llvmbot commented Jul 13, 2024 •

edited

Loading

beetrees commented Jul 13, 2024 •

edited

Loading