Skip to content

Commit eabd142

Browse files
beetreesbeetrees
committed
[APFloat] Fix IEEEFloat::addOrSubtractSignificand and IEEEFloat::normalize 
1 parent 7e3187e commit eabd142

File tree

3 files changed

+219
-16
lines changed

3 files changed

+219
-16
lines changed

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,8 @@ static constexpr fltCategory fcNaN = APFloatBase::fcNaN;
357357
static constexpr fltCategory fcNormal = APFloatBase::fcNormal;
358358
static constexpr fltCategory fcZero = APFloatBase::fcZero;
359359

360-
class IEEEFloat final {
360+
// Not final for testing purposes only.
361+
class IEEEFloat {
361362
public:
362363
/// \name Constructors
363364
/// @{
@@ -623,7 +624,11 @@ class IEEEFloat final {
623624

624625
integerPart addSignificand(const IEEEFloat &);
625626
integerPart subtractSignificand(const IEEEFloat &, integerPart);
627+
// Not private for testing purposes only.
628+
protected:
626629
lostFraction addOrSubtractSignificand(const IEEEFloat &, bool subtract);
630+
631+
private:
627632
lostFraction multiplySignificand(const IEEEFloat &, IEEEFloat,
628633
bool ignoreAddend = false);
629634
lostFraction multiplySignificand(const IEEEFloat&);
@@ -727,6 +732,8 @@ class IEEEFloat final {
727732
void copySignificand(const IEEEFloat &);
728733
void freeSignificand();
729734

735+
// Not private for testing purposes only.
736+
protected:
730737
/// Note: this must be the first data member.
731738
/// The semantics that this value obeys.
732739
const fltSemantics *semantics;

‎llvm/lib/Support/APFloat.cpp‎

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

1685-
if (omsb) {
1685+
// Only skip this `if` if the value is exactly zero.
1686+
if (omsb || lost_fraction != lfExactlyZero) {
16861687
/* OMSB is numbered from 1. We want to place it in the integer
16871688
bit numbered PRECISION if possible, with a compensating change in
16881689
the exponent. */
@@ -1864,7 +1865,7 @@ APFloat::opStatus IEEEFloat::addOrSubtractSpecials(const IEEEFloat &rhs,
18641865
/* Add or subtract two normal numbers. */
18651866
lostFraction IEEEFloat::addOrSubtractSignificand(const IEEEFloat &rhs,
18661867
bool subtract) {
1867-
integerPart carry;
1868+
integerPart carry = 0;
18681869
lostFraction lost_fraction;
18691870
int bits;
18701871

@@ -1882,35 +1883,54 @@ lostFraction IEEEFloat::addOrSubtractSignificand(const IEEEFloat &rhs,
18821883
"This floating point format does not support signed values");
18831884

18841885
IEEEFloat temp_rhs(rhs);
1886+
bool lost_fraction_is_from_rhs = false;
18851887

18861888
if (bits == 0)
18871889
lost_fraction = lfExactlyZero;
18881890
else if (bits > 0) {
18891891
lost_fraction = temp_rhs.shiftSignificandRight(bits - 1);
1892+
lost_fraction_is_from_rhs = true;
18901893
shiftSignificandLeft(1);
18911894
} else {
18921895
lost_fraction = shiftSignificandRight(-bits - 1);
18931896
temp_rhs.shiftSignificandLeft(1);
18941897
}
18951898

18961899
// Should we reverse the subtraction.
1897-
if (compareAbsoluteValue(temp_rhs) == cmpLessThan) {
1898-
carry = temp_rhs.subtractSignificand
1899-
(*this, lost_fraction != lfExactlyZero);
1900+
cmpResult cmp_result = compareAbsoluteValue(temp_rhs);
1901+
if (cmp_result == cmpLessThan) {
1902+
bool borrow =
1903+
lost_fraction != lfExactlyZero && !lost_fraction_is_from_rhs;
1904+
if (borrow) {
1905+
// The lost fraction is being subtracted, borrow from the significand
1906+
// and invert `lost_fraction`.
1907+
if (lost_fraction == lfLessThanHalf)
1908+
lost_fraction = lfMoreThanHalf;
1909+
else if (lost_fraction == lfMoreThanHalf)
1910+
lost_fraction = lfLessThanHalf;
1911+
}
1912+
carry = temp_rhs.subtractSignificand(*this, borrow);
19001913
copySignificand(temp_rhs);
19011914
sign = !sign;
1902-
} else {
1903-
carry = subtractSignificand
1904-
(temp_rhs, lost_fraction != lfExactlyZero);
1915+
} else if (cmp_result == cmpGreaterThan) {
1916+
bool borrow = lost_fraction != lfExactlyZero && lost_fraction_is_from_rhs;
1917+
if (borrow) {
1918+
// The lost fraction is being subtracted, borrow from the significand
1919+
// and invert `lost_fraction`.
1920+
if (lost_fraction == lfLessThanHalf)
1921+
lost_fraction = lfMoreThanHalf;
1922+
else if (lost_fraction == lfMoreThanHalf)
1923+
lost_fraction = lfLessThanHalf;
1924+
}
1925+
carry = subtractSignificand(temp_rhs, borrow);
1926+
} else { // cmpEqual
1927+
zeroSignificand();
1928+
if (lost_fraction != lfExactlyZero && lost_fraction_is_from_rhs) {
1929+
// rhs is slightly larger due to the lost fraction, flip the sign.
1930+
sign = !sign;
1931+
}
19051932
}
19061933

1907-
/* Invert the lost fraction - it was on the RHS and
1908-
subtracted. */
1909-
if (lost_fraction == lfLessThanHalf)
1910-
lost_fraction = lfMoreThanHalf;
1911-
else if (lost_fraction == lfMoreThanHalf)
1912-
lost_fraction = lfLessThanHalf;
1913-
19141934
/* The code above is intended to ensure that no borrow is
19151935
necessary. */
19161936
assert(!carry);

‎llvm/unittests/ADT/APFloatTest.cpp‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -560,6 +560,104 @@ 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+
638+
// Test cases from #104984
639+
{
640+
APFloat f1(0.24999998f);
641+
APFloat f2(2.3509885e-38f);
642+
APFloat f3(-1e-45f);
643+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
644+
EXPECT_EQ(5.87747e-39f, f1.convertToFloat());
645+
}
646+
{
647+
APFloat f1(4.4501477170144023e-308);
648+
APFloat f2(0.24999999999999997);
649+
APFloat f3(-8.475904604373977e-309);
650+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
651+
EXPECT_EQ(2.64946468816203e-309, f1.convertToDouble());
652+
}
653+
{
654+
APFloat f1(APFloat::IEEEhalf(), APInt(16, 0x8fffu));
655+
APFloat f2(APFloat::IEEEhalf(), APInt(16, 0x2bffu));
656+
APFloat f3(APFloat::IEEEhalf(), APInt(16, 0x0172u));
657+
f1.fusedMultiplyAdd(f2, f3, APFloat::rmNearestTiesToEven);
658+
EXPECT_EQ(0x808eu, f1.bitcastToAPInt().getZExtValue());
659+
}
660+
563661
// Test using only a single instance of APFloat.
564662
{
565663
APFloat F(1.5);
@@ -8089,4 +8187,82 @@ TEST(APFloatTest, Float4E2M1FNToFloat) {
80898187
EXPECT_TRUE(SmallestDenorm.isDenormal());
80908188
EXPECT_EQ(0x0.8p0, SmallestDenorm.convertToFloat());
80918189
}
8190+
8191+
TEST(APFloatTest, AddOrSubtractSignificand) {
8192+
class TestIEEEFloat : detail::IEEEFloat {
8193+
TestIEEEFloat(bool sign, APFloat::ExponentType exponent,
8194+
APFloat::integerPart significand)
8195+
: detail::IEEEFloat(1.0) {
8196+
// `addOrSubtractSignificand` only uses the sign, exponent and significand
8197+
this->sign = sign;
8198+
this->exponent = exponent;
8199+
this->significand.part = significand;
8200+
}
8201+
8202+
public:
8203+
static void runTest(bool subtract, bool lhsSign,
8204+
APFloat::ExponentType lhsExponent,
8205+
APFloat::integerPart lhsSignificand, bool rhsSign,
8206+
APFloat::ExponentType rhsExponent,
8207+
APFloat::integerPart rhsSignificand, bool expectedSign,
8208+
APFloat::ExponentType expectedExponent,
8209+
APFloat::integerPart expectedSignificand,
8210+
lostFraction expectedLoss) {
8211+
TestIEEEFloat lhs(lhsSign, lhsExponent, lhsSignificand);
8212+
TestIEEEFloat rhs(rhsSign, rhsExponent, rhsSignificand);
8213+
lostFraction resultLoss = lhs.addOrSubtractSignificand(rhs, subtract);
8214+
EXPECT_EQ(resultLoss, expectedLoss);
8215+
EXPECT_EQ(lhs.sign, expectedSign);
8216+
EXPECT_EQ(lhs.exponent, expectedExponent);
8217+
EXPECT_EQ(lhs.significand.part, expectedSignificand);
8218+
}
8219+
};
8220+
8221+
// Test cases are all combinations of:
8222+
// {equal exponents, LHS larger exponent, RHS larger exponent}
8223+
// {equal significands, LHS larger significand, RHS larger significand}
8224+
// {no loss, loss}
8225+
8226+
// Equal exponents (loss cannot occur as their is no shifting)
8227+
TestIEEEFloat::runTest(true, false, 1, 0x10, false, 1, 0x5, false, 1, 0xb,
8228+
lfExactlyZero);
8229+
TestIEEEFloat::runTest(false, false, -2, 0x20, true, -2, 0x20, false, -2, 0,
8230+
lfExactlyZero);
8231+
TestIEEEFloat::runTest(false, true, 3, 0x20, false, 3, 0x30, false, 3, 0x10,
8232+
lfExactlyZero);
8233+
8234+
// LHS larger exponent
8235+
// LHS significand greater after shitfing
8236+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x100, false, 6,
8237+
0x1e0, lfExactlyZero);
8238+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x101, false, 6,
8239+
0x1df, lfMoreThanHalf);
8240+
// Significands equal after shitfing
8241+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x1000, false, 6, 0,
8242+
lfExactlyZero);
8243+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x1001, true, 6, 0,
8244+
lfLessThanHalf);
8245+
// RHS significand greater after shitfing
8246+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x10000, true, 6,
8247+
0x1e00, lfExactlyZero);
8248+
TestIEEEFloat::runTest(true, false, 7, 0x100, false, 3, 0x10001, true, 6,
8249+
0x1e00, lfLessThanHalf);
8250+
8251+
// RHS larger exponent
8252+
// RHS significand greater after shitfing
8253+
TestIEEEFloat::runTest(true, false, 3, 0x100, false, 7, 0x100, true, 6, 0x1e0,
8254+
lfExactlyZero);
8255+
TestIEEEFloat::runTest(true, false, 3, 0x101, false, 7, 0x100, true, 6, 0x1df,
8256+
lfMoreThanHalf);
8257+
// Significands equal after shitfing
8258+
TestIEEEFloat::runTest(true, false, 3, 0x1000, false, 7, 0x100, false, 6, 0,
8259+
lfExactlyZero);
8260+
TestIEEEFloat::runTest(true, false, 3, 0x1001, false, 7, 0x100, false, 6, 0,
8261+
lfLessThanHalf);
8262+
// LHS significand greater after shitfing
8263+
TestIEEEFloat::runTest(true, false, 3, 0x10000, false, 7, 0x100, false, 6,
8264+
0x1e00, lfExactlyZero);
8265+
TestIEEEFloat::runTest(true, false, 3, 0x10001, false, 7, 0x100, false, 6,
8266+
0x1e00, lfLessThanHalf);
8267+
}
80928268
} // namespace

0 commit comments

Comments
 (0)