[PATCH] D70422: [APFloat] Fix fusedMultiplyAdd when `this` equals to `Addend`

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Support/APFloat.cpp:989
   bool ignored;
+  IEEEFloat extendedAddend(*semantics, 0);
 
----------------
ekatz wrote:
> foad wrote:
> > 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)?
> 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?
Sounds great if you can make it work. I wasn't sure exactly how to implement the "default to zero" part.


================
Comment at: llvm/lib/Support/APFloat.cpp:1031
 
-  if (addend && addend->isNonZero()) {
+  if (extendedAddend.isNonZero()) {
     // The intermediate result of the multiplication has "2 * precision"
----------------
ekatz wrote:
> foad wrote:
> > 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?
> Good observation, but adding/subtracting a negative zero, doesn't affect the sign (it is considered the same as positive zero).
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70422/new/

https://reviews.llvm.org/D70422