[llvm] [APFloat] Fix `IEEEFloat::addOrSubtractSignificand` and `IEEEFloat::normalize` (PR #98721)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 25 13:38:50 PDT 2024
https://github.com/beetrees updated https://github.com/llvm/llvm-project/pull/98721
>From 9b177513b93f861ce9bb24110eacff6b8aaef170 Mon Sep 17 00:00:00 2001
From: beetrees <b at beetr.ee>
Date: Sat, 13 Jul 2024 07:50:08 +0100
Subject: [PATCH] [APFloat] Fix `IEEEFloat::addOrSubtractSignificand` and
`IEEEFloat::normalize`
---
llvm/include/llvm/ADT/APFloat.h | 9 +-
llvm/lib/Support/APFloat.cpp | 50 +++++++---
llvm/unittests/ADT/APFloatTest.cpp | 150 +++++++++++++++++++++++++++++
3 files changed, 193 insertions(+), 16 deletions(-)
diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index db2fa480655c6..06e71ae1c5f40 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/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.
+class IEEEFloat : public APFloatBase {
public:
/// \name Constructors
/// @{
@@ -572,7 +573,11 @@ class IEEEFloat final : public APFloatBase {
integerPart addSignificand(const IEEEFloat &);
integerPart subtractSignificand(const IEEEFloat &, integerPart);
+ // Not private for testing purposes only.
+protected:
lostFraction addOrSubtractSignificand(const IEEEFloat &, bool subtract);
+
+private:
lostFraction multiplySignificand(const IEEEFloat &, IEEEFloat);
lostFraction multiplySignificand(const IEEEFloat&);
lostFraction divideSignificand(const IEEEFloat &);
@@ -668,6 +673,8 @@ class IEEEFloat final : public APFloatBase {
void copySignificand(const IEEEFloat &);
void freeSignificand();
+ // Not private for testing purposes only.
+protected:
/// Note: this must be the first data member.
/// The semantics that this value obeys.
const fltSemantics *semantics;
diff --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index 3664de71d06df..e162d97107864 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);
+ bool 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,40 @@ lostFraction IEEEFloat::addOrSubtractSignificand(const IEEEFloat &rhs,
}
// Should we reverse the subtraction.
- if (compareAbsoluteValue(temp_rhs) == cmpLessThan) {
- carry = temp_rhs.subtractSignificand
- (*this, lost_fraction != lfExactlyZero);
+ cmpResult cmp_result = compareAbsoluteValue(temp_rhs);
+ if (cmp_result == cmpLessThan) {
+ bool borrow =
+ lost_fraction != lfExactlyZero && !lost_fraction_is_from_rhs;
+ if (borrow) {
+ // The lost fraction is being subtracted, borrow from the significand
+ // and invert `lost_fraction`.
+ 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) {
+ bool borrow = lost_fraction != lfExactlyZero && lost_fraction_is_from_rhs;
+ if (borrow) {
+ // The lost fraction is being subtracted, borrow from the significand
+ // and invert `lost_fraction`.
+ 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..cfe3c6d98dae9 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -560,6 +560,81 @@ TEST(APFloatTest, FMA) {
EXPECT_EQ(-8.85242279E-41f, f1.convertToFloat());
}
+ // The `addOrSubtractSignificand` can be considered to have 9 possible cases
+ // when subtracting: all combinations of {cmpLessThan, cmpGreaterThan,
+ // cmpEqual} and {no loss, loss from lhs, loss from rhs}. Test each reachable
+ // case here.
+
+ // Regression test for failing the `assert(!carry)` in
+ // `addOrSubtractSignificand` and normalizing the exponent even when the
+ // significand is zero if there is a lost fraction.
+ // This tests cmpEqual, loss from lhs
+ {
+ 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 cmpGreaterThan, no loss
+ {
+ APFloat f1(2.0f);
+ APFloat f2(2.0f);
+ APFloat f3(-3.5f);
+ f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
+ EXPECT_EQ(0.5f, f1.convertToFloat());
+ }
+
+ // Test cmpLessThan, no loss
+ {
+ APFloat f1(2.0f);
+ APFloat f2(2.0f);
+ APFloat f3(-4.5f);
+ f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
+ EXPECT_EQ(-0.5f, f1.convertToFloat());
+ }
+
+ // Test cmpEqual, no loss
+ {
+ APFloat f1(2.0f);
+ APFloat f2(2.0f);
+ APFloat f3(-4.0f);
+ f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
+ EXPECT_EQ(0.0f, f1.convertToFloat());
+ }
+
+ // Test cmpLessThan, loss from lhs
+ {
+ APFloat f1(2.0000002f);
+ APFloat f2(2.0000002f);
+ APFloat f3(-32.0f);
+ f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
+ EXPECT_EQ(-27.999998f, f1.convertToFloat());
+ }
+
+ // Test cmpGreaterThan, loss from rhs
+ {
+ APFloat f1(1e10f);
+ APFloat f2(1e10f);
+ APFloat f3(-2.0000002f);
+ f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
+ EXPECT_EQ(1e20f, f1.convertToFloat());
+ }
+
+ // Test cmpGreaterThan, loss from lhs
+ {
+ APFloat f1(1e-36f);
+ APFloat f2(0.0019531252f);
+ APFloat f3(-1e-45f);
+ f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
+ EXPECT_EQ(1.953124e-39f, f1.convertToFloat());
+ }
+
+ // {cmpEqual, cmpLessThan} with loss from rhs can't occur for the usage in
+ // `fusedMultiplyAdd` as `multiplySignificand` normalises the MSB of lhs to
+ // one bit below the top.
+
// Test using only a single instance of APFloat.
{
APFloat F(1.5);
@@ -7440,4 +7515,79 @@ TEST(APFloatTest, Float4E2M1FNToFloat) {
EXPECT_TRUE(SmallestDenorm.isDenormal());
EXPECT_EQ(0x0.8p0, SmallestDenorm.convertToFloat());
}
+
+TEST(APFloatTest, AddOrSubtractSignificand) {
+ class TestIEEEFloat : detail::IEEEFloat {
+ TestIEEEFloat(bool sign, ExponentType exponent, integerPart significand)
+ : detail::IEEEFloat(1.0) {
+ // `addOrSubtractSignificand` only uses the sign, exponent and significand
+ this->sign = sign;
+ this->exponent = exponent;
+ this->significand.part = significand;
+ }
+
+ public:
+ static void runTest(bool subtract, bool lhsSign, ExponentType lhsExponent,
+ integerPart lhsSignificand, bool rhsSign,
+ ExponentType rhsExponent, integerPart rhsSignificand,
+ bool expectedSign, ExponentType expectedExponent,
+ integerPart expectedSignificand,
+ lostFraction expectedLoss) {
+ TestIEEEFloat lhs(lhsSign, lhsExponent, lhsSignificand);
+ TestIEEEFloat rhs(rhsSign, rhsExponent, rhsSignificand);
+ lostFraction resultLoss = lhs.addOrSubtractSignificand(rhs, subtract);
+ EXPECT_EQ(resultLoss, expectedLoss);
+ EXPECT_EQ(lhs.sign, expectedSign);
+ EXPECT_EQ(lhs.exponent, expectedExponent);
+ EXPECT_EQ(lhs.significand.part, expectedSignificand);
+ }
+ };
+
+ // Test cases are all combinations of:
+ // {equal exponents, LHS larger exponent, RHS larger exponent}
+ // {equal significands, LHS larger significand, RHS larger significand}
+ // {no loss, loss}
+
+ // Equal exponents (loss cannot occur as their is no shifting)
+ TestIEEEFloat::runTest(true, false, 1, 0x10, false, 1, 0x5, false, 1, 0xb,
+ lfExactlyZero);
+ TestIEEEFloat::runTest(false, false, -2, 0x20, true, -2, 0x20, false, -2, 0,
+ lfExactlyZero);
+ TestIEEEFloat::runTest(false, true, 3, 0x20, false, 3, 0x30, false, 3, 0x10,
+ lfExactlyZero);
+
+ // LHS larger exponent
+ // LHS significand greater after shitfing
+ TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x100, false, 6,
+ 0x1e0, lfExactlyZero);
+ TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x101, false, 6,
+ 0x1df, lfMoreThanHalf);
+ // Significands equal after shitfing
+ TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x1000, false, 6, 0,
+ lfExactlyZero);
+ TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x1001, true, 6, 0,
+ lfLessThanHalf);
+ // RHS significand greater after shitfing
+ TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x10000, true, 6,
+ 0x1e00, lfExactlyZero);
+ TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x10001, true, 6,
+ 0x1e00, lfLessThanHalf);
+
+ // RHS larger exponent
+ // RHS significand greater after shitfing
+ TestIEEEFloat::runTest(true, false, 3, 0x100, false, 7, 0x100, true, 6, 0x1e0,
+ lfExactlyZero);
+ TestIEEEFloat::runTest(true, false, 3, 0x101, false, 7, 0x100, true, 6, 0x1df,
+ lfMoreThanHalf);
+ // Significands equal after shitfing
+ TestIEEEFloat::runTest(true, false, 3, 0x1000, false, 7, 0x100, false, 6, 0,
+ lfExactlyZero);
+ TestIEEEFloat::runTest(true, false, 3, 0x1001, false, 7, 0x100, false, 6, 0,
+ lfLessThanHalf);
+ // LHS significand greater after shitfing
+ TestIEEEFloat::runTest(true, false, 3, 0x10000, false, 7, 0x100, false, 6,
+ 0x1e00, lfExactlyZero);
+ TestIEEEFloat::runTest(true, false, 3, 0x10001, false, 7, 0x100, false, 6,
+ 0x1e00, lfLessThanHalf);
+}
} // namespace
More information about the llvm-commits
mailing list