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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Lang Hames
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.

Cheers,
Lang.



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

APFloat_FMA_truncated_zero_sign.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Mehdi Amini-2
Hi Lang,

On Jan 1, 2015, at 5:57 PM, Lang Hames <[hidden email]> 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


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

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

Lang Hames
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 <[hidden email]> wrote:
Hi Lang,

On Jan 1, 2015, at 5:57 PM, Lang Hames <[hidden email]> 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



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

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

Mehdi Amini-2
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 <[hidden email]> 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 <[hidden email]> wrote:
Hi Lang,

On Jan 1, 2015, at 5:57 PM, Lang Hames <[hidden email]> 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




_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi Lang,

On Jan 1, 2015, at 5:57 PM, Lang Hames <[hidden email]> 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





_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev