[LLVMdev] [PATCH] [ADT] APFloat - Fix sign handling for FMA results that truncate to zero.

[Lang Hames]

Committed with additional tests and comments as requested in r225123.

Thanks for the review Mehdi.

- Lang.

On Fri, Jan 2, 2015 at 7:26 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:

> OK I see now, your change enforce the “(exactly)” in the comment.
>
> The commit LGTM.
>
> Some comments:
>
> I would still love to see a comment in the unit test file (as it is done
> earlier in the file for the other tests).
>
> Also the unit test does not test the rmTowardNegative case. It seems to
> be never tested by the validation. I changed
>
> sign = (rounding_mode == rmTowardNegative);
>
> for
>
> sign = 0;
>
> and ninja check-all is still passing.
>
> Mehdi
>
>
> On Jan 2, 2015, at 4:59 PM, Lang Hames <lhames at gmail.com> wrote:
>
> Hi Mehdi,
>
> Thanks for the feedback. I think the comment is fine - the problem is just
> that the original conditional didn't exactly match it. With the addition of
> the underflow check I believe the condition now matches the comment: The
> sign is only tweaked if the result of the addition is exactly zero, rather
> than a small result that truncates to zero.
>
> CC'ing llvm-commits, where this email should have gone in the first place.
> :)
>
> Cheers,
> Lang.
>
> On Thu, Jan 1, 2015 at 10:56 PM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>> Hi Lang,
>>
>> On Jan 1, 2015, at 5:57 PM, Lang Hames <lhames at gmail.com> wrote:
>>
>> Hi All,
>>
>> APFloat::fusedMultiplyAdd currently computes the wrong signed zero when
>> small negative results are truncated back to zero in standard precision.
>> The following snippet handles the signedness in fusedMultiplyAdd:
>>
>> /* If two numbers add (exactly) to zero, IEEE 754 decrees it is a
>>    positive zero unless rounding to minus infinity, except that
>>
>>    adding two like-signed zeroes gives that zero.  */
>> if (category == fcZero && sign != addend.sign)
>>   sign = (rounding_mode == rmTowardNegative);
>>
>> The test "category == fcZero" tells us that the result was zero after
>> rounding back down to standard precision, but since the addition is carried
>> out in extended precision this doesn't guarantee that the result of the
>> addition was exactly zero (so the comment text may not apply). The attached
>> patch adds a check for underflow during truncation which ensures the
>> correct signedness of the result.
>>
>>
>>
>> According to what you describe ("the comment text may not apply"), I feel
>> that the comment might be updated as well.
>>
>> Best,
>>
>> Mehdi
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150103/d65f5dbf/attachment.html>