Skip to content

Commit 9b17751

Browse files
beetreesbeetrees
committed
[APFloat] Fix IEEEFloat::addOrSubtractSignificand and IEEEFloat::normalize 
1 parent 7d1b6b2 commit 9b17751

File tree

3 files changed

+193
-16
lines changed

3 files changed

+193
-16
lines changed

‎llvm/include/llvm/ADT/APFloat.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,8 @@ struct APFloatBase {
306306

307307
namespace detail {
308308

309-
class IEEEFloat final : public APFloatBase {
309+
// Not final for testing purposes only.
310+
class IEEEFloat : public APFloatBase {
310311
public:
311312
/// \name Constructors
312313
/// @{
@@ -572,7 +573,11 @@ class IEEEFloat final : public APFloatBase {
572573

573574
integerPart addSignificand(const IEEEFloat &);
574575
integerPart subtractSignificand(const IEEEFloat &, integerPart);
576+
// Not private for testing purposes only.
577+
protected:
575578
lostFraction addOrSubtractSignificand(const IEEEFloat &, bool subtract);
579+
580+
private:
576581
lostFraction multiplySignificand(const IEEEFloat &, IEEEFloat);
577582
lostFraction multiplySignificand(const IEEEFloat&);
578583
lostFraction divideSignificand(const IEEEFloat &);
@@ -668,6 +673,8 @@ class IEEEFloat final : public APFloatBase {
668673
void copySignificand(const IEEEFloat &);
669674
void freeSignificand();
670675

676+
// Not private for testing purposes only.
677+
protected:
671678
/// Note: this must be the first data member.
672679
/// The semantics that this value obeys.
673680
const fltSemantics *semantics;

‎llvm/lib/Support/APFloat.cpp‎

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1607,7 +1607,8 @@ IEEEFloat::opStatus IEEEFloat::normalize(roundingMode rounding_mode,
16071607
/* Before rounding normalize the exponent of fcNormal numbers. */
16081608
omsb = significandMSB() + 1;
16091609

1610-
if (omsb) {
1610+
// Only skip this `if` if the value is exactly zero.
1611+
if (omsb || lost_fraction != lfExactlyZero) {
16111612
/* OMSB is numbered from 1. We want to place it in the integer
16121613
bit numbered PRECISION if possible, with a compensating change in
16131614
the exponent. */
@@ -1782,7 +1783,7 @@ IEEEFloat::opStatus IEEEFloat::addOrSubtractSpecials(const IEEEFloat &rhs,
17821783
/* Add or subtract two normal numbers. */
17831784
lostFraction IEEEFloat::addOrSubtractSignificand(const IEEEFloat &rhs,
17841785
bool subtract) {
1785-
integerPart carry;
1786+
integerPart carry = 0;
17861787
lostFraction lost_fraction;
17871788
int bits;
17881789

@@ -1796,35 +1797,54 @@ lostFraction IEEEFloat::addOrSubtractSignificand(const IEEEFloat &rhs,
17961797
/* Subtraction is more subtle than one might naively expect. */
17971798
if (subtract) {
17981799
IEEEFloat temp_rhs(rhs);
1800+
bool lost_fraction_is_from_rhs = false;
17991801

18001802
if (bits == 0)
18011803
lost_fraction = lfExactlyZero;
18021804
else if (bits > 0) {
18031805
lost_fraction = temp_rhs.shiftSignificandRight(bits - 1);
1806+
lost_fraction_is_from_rhs = true;
18041807
shiftSignificandLeft(1);
18051808
} else {
18061809
lost_fraction = shiftSignificandRight(-bits - 1);
18071810
temp_rhs.shiftSignificandLeft(1);
18081811
}
18091812

18101813
// Should we reverse the subtraction.
1811-
if (compareAbsoluteValue(temp_rhs) == cmpLessThan) {
1812-
carry = temp_rhs.subtractSignificand
1813-
(*this, lost_fraction != lfExactlyZero);
1814+
cmpResult cmp_result = compareAbsoluteValue(temp_rhs);
1815+
if (cmp_result == cmpLessThan) {
1816+
bool borrow =
1817+
lost_fraction != lfExactlyZero && !lost_fraction_is_from_rhs;
1818+
if (borrow) {
1819+
// The lost fraction is being subtracted, borrow from the significand
1820+
// and invert `lost_fraction`.
1821+
if (lost_fraction == lfLessThanHalf)
1822+
lost_fraction = lfMoreThanHalf;
1823+
else if (lost_fraction == lfMoreThanHalf)
1824+
lost_fraction = lfLessThanHalf;
1825+
}
1826+
carry = temp_rhs.subtractSignificand(*this, borrow);
18141827
copySignificand(temp_rhs);
18151828
sign = !sign;
1816-
} else {
1817-
carry = subtractSignificand
1818-
(temp_rhs, lost_fraction != lfExactlyZero);
1829+
} else if (cmp_result == cmpGreaterThan) {
1830+
bool borrow = lost_fraction != lfExactlyZero && lost_fraction_is_from_rhs;
1831+
if (borrow) {
1832+
// The lost fraction is being subtracted, borrow from the significand
1833+
// and invert `lost_fraction`.
1834+
if (lost_fraction == lfLessThanHalf)
1835+
lost_fraction = lfMoreThanHalf;
1836+
else if (lost_fraction == lfMoreThanHalf)
1837+
lost_fraction = lfLessThanHalf;
1838+
}
1839+
carry = subtractSignificand(temp_rhs, borrow);
1840+
} else { // cmpEqual
1841+
zeroSignificand();
1842+
if (lost_fraction != lfExactlyZero && lost_fraction_is_from_rhs) {
1843+
// rhs is slightly larger due to the lost fraction, flip the sign.
1844+
sign = !sign;
1845+
}
18191846
}
18201847

1821-
/* Invert the lost fraction - it was on the RHS and
1822-
subtracted. */
1823-
if (lost_fraction == lfLessThanHalf)
1824-
lost_fraction = lfMoreThanHalf;
1825-
else if (lost_fraction == lfMoreThanHalf)
1826-
lost_fraction = lfLessThanHalf;
1827-
18281848
/* The code above is intended to ensure that no borrow is
18291849
necessary. */
18301850
assert(!carry);

‎llvm/unittests/ADT/APFloatTest.cpp‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -560,6 +560,81 @@ TEST(APFloatTest, FMA) {
560560
EXPECT_EQ(-8.85242279E-41f, f1.convertToFloat());
561561
}
562562

563+
// The `addOrSubtractSignificand` can be considered to have 9 possible cases
564+
// when subtracting: all combinations of {cmpLessThan, cmpGreaterThan,
565+
// cmpEqual} and {no loss, loss from lhs, loss from rhs}. Test each reachable
566+
// case here.
567+
568+
// Regression test for failing the `assert(!carry)` in
569+
// `addOrSubtractSignificand` and normalizing the exponent even when the
570+
// significand is zero if there is a lost fraction.
571+
// This tests cmpEqual, loss from lhs
572+
{
573+
APFloat f1(-1.4728589E-38f);
574+
APFloat f2(3.7105144E-6f);
575+
APFloat f3(5.5E-44f);
576+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
577+
EXPECT_EQ(-0.0f, f1.convertToFloat());
578+
}
579+
580+
// Test cmpGreaterThan, no loss
581+
{
582+
APFloat f1(2.0f);
583+
APFloat f2(2.0f);
584+
APFloat f3(-3.5f);
585+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
586+
EXPECT_EQ(0.5f, f1.convertToFloat());
587+
}
588+
589+
// Test cmpLessThan, no loss
590+
{
591+
APFloat f1(2.0f);
592+
APFloat f2(2.0f);
593+
APFloat f3(-4.5f);
594+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
595+
EXPECT_EQ(-0.5f, f1.convertToFloat());
596+
}
597+
598+
// Test cmpEqual, no loss
599+
{
600+
APFloat f1(2.0f);
601+
APFloat f2(2.0f);
602+
APFloat f3(-4.0f);
603+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
604+
EXPECT_EQ(0.0f, f1.convertToFloat());
605+
}
606+
607+
// Test cmpLessThan, loss from lhs
608+
{
609+
APFloat f1(2.0000002f);
610+
APFloat f2(2.0000002f);
611+
APFloat f3(-32.0f);
612+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
613+
EXPECT_EQ(-27.999998f, f1.convertToFloat());
614+
}
615+
616+
// Test cmpGreaterThan, loss from rhs
617+
{
618+
APFloat f1(1e10f);
619+
APFloat f2(1e10f);
620+
APFloat f3(-2.0000002f);
621+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
622+
EXPECT_EQ(1e20f, f1.convertToFloat());
623+
}
624+
625+
// Test cmpGreaterThan, loss from lhs
626+
{
627+
APFloat f1(1e-36f);
628+
APFloat f2(0.0019531252f);
629+
APFloat f3(-1e-45f);
630+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
631+
EXPECT_EQ(1.953124e-39f, f1.convertToFloat());
632+
}
633+
634+
// {cmpEqual, cmpLessThan} with loss from rhs can't occur for the usage in
635+
// `fusedMultiplyAdd` as `multiplySignificand` normalises the MSB of lhs to
636+
// one bit below the top.
637+
563638
// Test using only a single instance of APFloat.
564639
{
565640
APFloat F(1.5);
@@ -7440,4 +7515,79 @@ TEST(APFloatTest, Float4E2M1FNToFloat) {
74407515
EXPECT_TRUE(SmallestDenorm.isDenormal());
74417516
EXPECT_EQ(0x0.8p0, SmallestDenorm.convertToFloat());
74427517
}
7518+
7519+
TEST(APFloatTest, AddOrSubtractSignificand) {
7520+
class TestIEEEFloat : detail::IEEEFloat {
7521+
TestIEEEFloat(bool sign, ExponentType exponent, integerPart significand)
7522+
: detail::IEEEFloat(1.0) {
7523+
// `addOrSubtractSignificand` only uses the sign, exponent and significand
7524+
this->sign = sign;
7525+
this->exponent = exponent;
7526+
this->significand.part = significand;
7527+
}
7528+
7529+
public:
7530+
static void runTest(bool subtract, bool lhsSign, ExponentType lhsExponent,
7531+
integerPart lhsSignificand, bool rhsSign,
7532+
ExponentType rhsExponent, integerPart rhsSignificand,
7533+
bool expectedSign, ExponentType expectedExponent,
7534+
integerPart expectedSignificand,
7535+
lostFraction expectedLoss) {
7536+
TestIEEEFloat lhs(lhsSign, lhsExponent, lhsSignificand);
7537+
TestIEEEFloat rhs(rhsSign, rhsExponent, rhsSignificand);
7538+
lostFraction resultLoss = lhs.addOrSubtractSignificand(rhs, subtract);
7539+
EXPECT_EQ(resultLoss, expectedLoss);
7540+
EXPECT_EQ(lhs.sign, expectedSign);
7541+
EXPECT_EQ(lhs.exponent, expectedExponent);
7542+
EXPECT_EQ(lhs.significand.part, expectedSignificand);
7543+
}
7544+
};
7545+
7546+
// Test cases are all combinations of:
7547+
// {equal exponents, LHS larger exponent, RHS larger exponent}
7548+
// {equal significands, LHS larger significand, RHS larger significand}
7549+
// {no loss, loss}
7550+
7551+
// Equal exponents (loss cannot occur as their is no shifting)
7552+
TestIEEEFloat::runTest(true, false, 1, 0x10, false, 1, 0x5, false, 1, 0xb,
7553+
lfExactlyZero);
7554+
TestIEEEFloat::runTest(false, false, -2, 0x20, true, -2, 0x20, false, -2, 0,
7555+
lfExactlyZero);
7556+
TestIEEEFloat::runTest(false, true, 3, 0x20, false, 3, 0x30, false, 3, 0x10,
7557+
lfExactlyZero);
7558+
7559+
// LHS larger exponent
7560+
// LHS significand greater after shitfing
7561+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x100, false, 6,
7562+
0x1e0, lfExactlyZero);
7563+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x101, false, 6,
7564+
0x1df, lfMoreThanHalf);
7565+
// Significands equal after shitfing
7566+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x1000, false, 6, 0,
7567+
lfExactlyZero);
7568+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x1001, true, 6, 0,
7569+
lfLessThanHalf);
7570+
// RHS significand greater after shitfing
7571+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x10000, true, 6,
7572+
0x1e00, lfExactlyZero);
7573+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x10001, true, 6,
7574+
0x1e00, lfLessThanHalf);
7575+
7576+
// RHS larger exponent
7577+
// RHS significand greater after shitfing
7578+
TestIEEEFloat::runTest(true, false, 3, 0x100, false, 7, 0x100, true, 6, 0x1e0,
7579+
lfExactlyZero);
7580+
TestIEEEFloat::runTest(true, false, 3, 0x101, false, 7, 0x100, true, 6, 0x1df,
7581+
lfMoreThanHalf);
7582+
// Significands equal after shitfing
7583+
TestIEEEFloat::runTest(true, false, 3, 0x1000, false, 7, 0x100, false, 6, 0,
7584+
lfExactlyZero);
7585+
TestIEEEFloat::runTest(true, false, 3, 0x1001, false, 7, 0x100, false, 6, 0,
7586+
lfLessThanHalf);
7587+
// LHS significand greater after shitfing
7588+
TestIEEEFloat::runTest(true, false, 3, 0x10000, false, 7, 0x100, false, 6,
7589+
0x1e00, lfExactlyZero);
7590+
TestIEEEFloat::runTest(true, false, 3, 0x10001, false, 7, 0x100, false, 6,
7591+
0x1e00, lfLessThanHalf);
7592+
}
74437593
} // namespace

0 commit comments

Comments
 (0)