[APFloat] Fix fusedMultiplyAdd when `this` equals to `Addend`

[ekatz]

Summary

The arguments to fusedMultiplyAdd are passed by reference. We must save the Addend value on the beginning of the function, before we modify this, as they may be the same reference.

Fix PR44051.

[hfinkel]

LGTM

[foad]

Typo in commit message: "this this"

llvm/lib/Support/APFloat.cpp

989

Marginally more efficient to omit the , 0 here, to use the one-argument constructor.

Since we are unconditionally constructing the addend here anyway, would it make more sense to force all callers to pass in an addend (using zero if they're not interested in it)?

1031

Is it safe to skip this code when the addend is zero? If the result of the multiply is also zero, adding zero could affect the sign of the result, couldn't it?

[ekatz]

llvm/lib/Support/APFloat.cpp

989

Correct. I'll change the constructor to the single parameter version.

Regarding the construction of the addend, that is a good idea.
I suggest making addend passed by value, with default to zero (keep in mind that this function is private, so there is no API change).
What do you think?

1031

Good observation, but adding/subtracting a negative zero, doesn't affect the sign (it is considered the same as positive zero).

[foad]

llvm/lib/Support/APFloat.cpp

989

Sounds great if you can make it work. I wasn't sure exactly how to implement the "default to zero" part.

1031

A couple of cases where adding zero changes the sign, accoerding to the comment at the end of IEEEFloat::addOrSubtract:
-0 + +0 = +0 (except if rounding to -inf)
+0 + -0 = -0 (if rounding to -inf)
(I realise this code is pre-existing, so no need to address it as part of the current patch.)

[ekatz]

llvm/lib/Support/APFloat.cpp

1031

Good catch! Those corner cases never end...
I'll open a new bug for it.

[ekatz]

Pass added parameter by value (instead of - by ref), and have a default value of zero.

[ekatz]

Note that this is a safe overload, as multiplySignificand is private.

llvm/lib/Support/APFloat.cpp

lostFraction IEEEFloat::multiplySignificand(const IEEEFloat &rhs,
                                             const IEEEFloat *addend) {
   unsigned int omsb;        // One, not zero, based MSB.
   unsigned int partsCount, newPartsCount, precision;
   integerPart *lhsSignificand;
   integerPart scratch[4];
   integerPart *fullSignificand;
   lostFraction lost_fraction;
   bool ignored;
+  IEEEFloat extendedAddend(*semantics, 0);
 
   assert(semantics == rhs.semantics);
 
+  // Save the addend now, before we change our properties, as it may be equal to
+  // 'this' pointer.
+  if (addend)
+    extendedAddend = *addend;
+
   precision = semantics->precision;
 
   // Allocate space for twice as many bits as the original significand, plus one
   // extra bit for the addition to overflow into.
   newPartsCount = partCountForBits(precision * 2 + 1);
 
   if (newPartsCount > 4)
     fullSignificand = new integerPart[newPartsCount];
lostFraction IEEEFloat::multiplySignificand(const IEEEFloat &rhs,
   // the result of multiplication is:
   //   *this = c48 c47 c46 . c45 ... c0 * 2^(e1+e2)
   // Note that there are three significant bits at the left-hand side of the
   // radix point: two for the multiplication, and an overflow bit for the
   // addition (that will always be zero at this point). Move the radix point
   // toward left by two bits, and adjust exponent accordingly.
   exponent += 2;
 
-  if (addend && addend->isNonZero()) {
+  if (extendedAddend.isNonZero()) {
     // The intermediate result of the multiplication has "2 * precision"
     // signicant bit; adjust the addend to be consistent with mul result.
     //
     Significand savedSignificand = significand;
     const fltSemantics *savedSemantics = semantics;
     fltSemantics extendedSemantics;
     opStatus status;
     unsigned int extendedPrecision;
if (extendedAddend.isNonZero()) {
     extendedSemantics.precision = extendedPrecision;
 
     if (newPartsCount == 1)
       significand.part = fullSignificand[0];
     else
       significand.parts = fullSignificand;
     semantics = &extendedSemantics;
 
-    IEEEFloat extendedAddend(*addend);
     status = extendedAddend.convert(extendedSemantics, rmTowardZero, &ignored);
     assert(status == opOK);
     (void)status;
 
     // Shift the significand of the addend right by one bit. This guarantees
     // that the high bit of the significand is zero (same as fullSignificand),
     // so the addition will overflow (if it does overflow at all) into the top bit.
     lost_fraction = extendedAddend.shiftSignificandRight(1);

llvm/unittests/ADT/APFloatTest.cpp

// Test x87 extended precision case from http://llvm.org/PR20728.
     APFloat A(APFloat::x87DoubleExtended(), 3.0);
 
     bool losesInfo = false;
     M1.fusedMultiplyAdd(M1, A, APFloat::rmNearestTiesToEven);
     M1.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
     EXPECT_FALSE(losesInfo);
     EXPECT_EQ(4.0f, M1.convertToFloat());
   }
+
+  // Test using only a single instance of APFloat.
+  {
+    APFloat F(1.5);
+
+    F.fusedMultiplyAdd(F, F, APFloat::rmNearestTiesToEven);
+    EXPECT_EQ(3.75, F.convertToDouble());
+  }
 }
 
 TEST(APFloatTest, MinNum) {
   APFloat f1(1.0);
   APFloat f2(2.0);
   APFloat nan = APFloat::getNaN(APFloat::IEEEdouble());
 
   EXPECT_EQ(1.0, minnum(f1, f2).convertToDouble());