[PATCH] Fix silent overflow bug in APFloat::multiplySignificand (PR20728).

[Lang Hames]

Thanks Steve. Committed in r222374.

- Lang.

On Tue, Nov 18, 2014 at 5:22 PM, Stephen Canon <scanon at apple.com> wrote:

> This looks reasonable to me.
>
> – Steve
>
> On Nov 18, 2014, at 8:13 PM, Lang Hames <lhames at gmail.com> wrote:
>
> Hi All,
>
> I've been digging in to http://llvm.org/PR20728 and I've discovered the
> root cause of the bug detailed there. The short version is that on x86 the
> following call should constant-fold to 4.0L:
>
> fmal(1.0L, 1.0L, 3.0L)
>
> However, it currently constant folds to 0.0L.
>
> Here's what goes wrong: The call is constant folded using
> APFloat::fusedMultiplyAdd, but the real work is done by
> APFloat::multiplySignificand. APFloat::multiplySignificand takes an RHS for
> the multiplication and an optional addend. The method allocates a 2x
> precision intermediate significand to hold the result of both operations.
> This is sufficient to hold the result of the multiplication without
> overflowing, but if the MSB of the multiplication result and the addend are
> both in the top bits of their respective significands, the addition
> operation will overflow. For reasons too perverse to detail here (read the
> bug for the gory details) this overflow is not detected, and
> multiplySignificand happily returns bogus results. In the case of the test
> case above, you end up with:
>
>     0100...0  (1.0)
> +   1100...0  (3.0)
> ---------------------
> 1 | 0000...0  (0.0!)
>
> The attached patch fixes this bug by holding the intermediate result in 2
> * precision + 1 bits, and arranging for the top bit of the addend to be
> zero (by shifting right by 1). This means that the addition no longer
> overflows, but simply carries into the top bit.
>
> This patch passes the regression test suite and the nightly test suite on
> x86-64, so that's a good start. Are there any APFloat experts out there who
> are able to offer comment on the approach I've taken though? It seems
> superficially sane, but this is floating point code - the appearance of
> sanity may be purely coincidental.
>
> Cheers,
> Lang.
> <APFloat_multiplySignificand_fix.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141119/416ed730/attachment.html>