Patchwork [V2,11/18] softfloat: Fix float64_to_uint32

login
register
mail settings
Submitter Tom Musta
Date Dec. 11, 2013, 7:16 p.m.
Message ID <1386789398-5239-12-git-send-email-tommusta@gmail.com>
Download mbox | patch
Permalink /patch/300305/
State New
Headers show

Comments

Tom Musta - Dec. 11, 2013, 7:16 p.m.
The float64_to_uint32 has several flaws:

 - for numbers between 2**32 and 2**64, the inexact exception flag
   may get incorrectly set.  In this case, only the invalid flag
   should be set.

       test pattern: 425F81378DC0CD1F / 0x1.f81378dc0cd1fp+38

 - for numbers between 2**63 and 2**64, incorrect results may
   be produced:

       test pattern: 43EAAF73F1F0B8BD / 0x1.aaf73f1f0b8bdp+63

This patch re-implements float64_to_uint32 to re-use the
float64_to_uint64 routine (instead of float64_to_int64).  For the
saturation case, the inexact bit is explicitly cleared before raising
the invalid flag.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
 fpu/softfloat.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
Peter Maydell - Dec. 11, 2013, 7:53 p.m.
On 11 December 2013 19:16, Tom Musta <tommusta@gmail.com> wrote:
>  uint32 float64_to_uint32( float64 a STATUS_PARAM )
>  {
> -    int64_t v;
> +    uint64_t v;
>      uint32 res;
>
> -    v = float64_to_int64(a STATUS_VAR);
> -    if (v < 0) {
> -        res = 0;
> -        float_raise( float_flag_invalid STATUS_VAR);
> -    } else if (v > 0xffffffff) {
> +    v = float64_to_uint64(a STATUS_VAR);
> +    if (v > 0xffffffff) {
>          res = 0xffffffff;
> +        STATUS(float_exception_flags) &= ~float_flag_inexact;

The IEEE exception flags are cumulative (ie never get cleared
except by guest program explicit request); this change means
that if a previous operation raised the inexact flag you've just
lost that.

thanks
-- PMM
Tom Musta - Dec. 11, 2013, 8:39 p.m.
On 12/11/2013 1:53 PM, Peter Maydell wrote:
> On 11 December 2013 19:16, Tom Musta <tommusta@gmail.com> wrote:
>>  uint32 float64_to_uint32( float64 a STATUS_PARAM )
>>  {
>> -    int64_t v;
>> +    uint64_t v;
>>      uint32 res;
>>
>> -    v = float64_to_int64(a STATUS_VAR);
>> -    if (v < 0) {
>> -        res = 0;
>> -        float_raise( float_flag_invalid STATUS_VAR);
>> -    } else if (v > 0xffffffff) {
>> +    v = float64_to_uint64(a STATUS_VAR);
>> +    if (v > 0xffffffff) {
>>          res = 0xffffffff;
>> +        STATUS(float_exception_flags) &= ~float_flag_inexact;
> 
> The IEEE exception flags are cumulative (ie never get cleared
> except by guest program explicit request); this change means
> that if a previous operation raised the inexact flag you've just
> lost that.
> 
> thanks
> -- PMM
> 
Thank you, Peter.  I will fix.
Peter Maydell - Dec. 17, 2013, 5:45 p.m.
On 11 December 2013 20:39, Tom Musta <tommusta@gmail.com> wrote:
> On 12/11/2013 1:53 PM, Peter Maydell wrote:
>> On 11 December 2013 19:16, Tom Musta <tommusta@gmail.com> wrote:
>>>  uint32 float64_to_uint32( float64 a STATUS_PARAM )
>>>  {
>>> -    int64_t v;
>>> +    uint64_t v;
>>>      uint32 res;
>>>
>>> -    v = float64_to_int64(a STATUS_VAR);
>>> -    if (v < 0) {
>>> -        res = 0;
>>> -        float_raise( float_flag_invalid STATUS_VAR);
>>> -    } else if (v > 0xffffffff) {
>>> +    v = float64_to_uint64(a STATUS_VAR);
>>> +    if (v > 0xffffffff) {
>>>          res = 0xffffffff;
>>> +        STATUS(float_exception_flags) &= ~float_flag_inexact;
>>
>> The IEEE exception flags are cumulative (ie never get cleared
>> except by guest program explicit request); this change means
>> that if a previous operation raised the inexact flag you've just
>> lost that.
>>
>> thanks
>> -- PMM
>>
> Thank you, Peter.  I will fix.

I'm partway through fixing this bug in an implementation of
float*_to_uint16 which the ARM AArch64 needs. I think the
cleanest approach to this looks like this:

uint32 float64_to_uint32( float64 a STATUS_PARAM )
{
    int64_t v;
    uint32 res;
    int old_exc_flags = get_float_exception_flags(status);

    v = float64_to_uint64(a STATUS_VAR);
    if (v > 0xffffffff) {
        res = 0xffffffff;
    } else {
        return v;
    }
    set_float_exception_flags(old_exc_flags);
    float_raise(float_flag_invalid STATUS_VAR);
    return res;
}

thanks
-- PMM
Peter Maydell - Dec. 17, 2013, 7:32 p.m.
On 17 December 2013 17:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> I'm partway through fixing this bug in an implementation of
> float*_to_uint16 which the ARM AArch64 needs. I think the
> cleanest approach to this looks like this:
>
> uint32 float64_to_uint32( float64 a STATUS_PARAM )
> {
>     int64_t v;
>     uint32 res;
>     int old_exc_flags = get_float_exception_flags(status);
>
>     v = float64_to_uint64(a STATUS_VAR);
>     if (v > 0xffffffff) {
>         res = 0xffffffff;
>     } else {
>         return v;
>     }
>     set_float_exception_flags(old_exc_flags);

...should be
    set_float_exception_flags(old_exc_flags, status);

>     float_raise(float_flag_invalid STATUS_VAR);
>     return res;
> }

thanks
-- PMM
Tom Musta - Dec. 18, 2013, 5:32 p.m.
On 12/17/2013 11:45 AM, Peter Maydell wrote:
> On 11 December 2013 20:39, Tom Musta <tommusta@gmail.com> wrote:
>> On 12/11/2013 1:53 PM, Peter Maydell wrote:
>>> On 11 December 2013 19:16, Tom Musta <tommusta@gmail.com> wrote:
>>>>  uint32 float64_to_uint32( float64 a STATUS_PARAM )
>>>>  {
>>>> -    int64_t v;
>>>> +    uint64_t v;
>>>>      uint32 res;
>>>>
>>>> -    v = float64_to_int64(a STATUS_VAR);
>>>> -    if (v < 0) {
>>>> -        res = 0;
>>>> -        float_raise( float_flag_invalid STATUS_VAR);
>>>> -    } else if (v > 0xffffffff) {
>>>> +    v = float64_to_uint64(a STATUS_VAR);
>>>> +    if (v > 0xffffffff) {
>>>>          res = 0xffffffff;
>>>> +        STATUS(float_exception_flags) &= ~float_flag_inexact;
>>>
>>> The IEEE exception flags are cumulative (ie never get cleared
>>> except by guest program explicit request); this change means
>>> that if a previous operation raised the inexact flag you've just
>>> lost that.
>>>
>>> thanks
>>> -- PMM
>>>
>> Thank you, Peter.  I will fix.
> 
> I'm partway through fixing this bug in an implementation of
> float*_to_uint16 which the ARM AArch64 needs. I think the
> cleanest approach to this looks like this:
> 
> uint32 float64_to_uint32( float64 a STATUS_PARAM )
> {
>     int64_t v;
>     uint32 res;
>     int old_exc_flags = get_float_exception_flags(status);
> 
>     v = float64_to_uint64(a STATUS_VAR);
>     if (v > 0xffffffff) {
>         res = 0xffffffff;
>     } else {
>         return v;
>     }
>     set_float_exception_flags(old_exc_flags);
>     float_raise(float_flag_invalid STATUS_VAR);
>     return res;
> }
> 
> thanks
> -- PMM
> 

This seems to assume that the only case where flags could be set in
float64_to_uint32 is the case where a large result is returned. Is
this really the case?

I was thinking something like this:

uint32 float64_to_uint32( float64 a STATUS_PARAM )
{
    uint64_t v;
    uint32 res;
    int inexact = (STATUS(float_exception_flags) & float_flag_inexact) != 0;

    v = float64_to_uint64(a STATUS_VAR);
    if (v > 0xffffffff) {
        res = 0xffffffff;
        /* If the inexact flag was set during this operation, then clear it. */
        if (!inexact) {
            STATUS(float_exception_flags) &= ~float_flag_inexact;
        }
        float_raise( float_flag_invalid STATUS_VAR);
    } else {
        res = v;
    }
    return res;
}
Peter Maydell - Dec. 18, 2013, 6:03 p.m.
On 18 December 2013 17:32, Tom Musta <tommusta@gmail.com> wrote:
> On 12/17/2013 11:45 AM, Peter Maydell wrote:
>> I'm partway through fixing this bug in an implementation of
>> float*_to_uint16 which the ARM AArch64 needs. I think the
>> cleanest approach to this looks like this:
>>
>> uint32 float64_to_uint32( float64 a STATUS_PARAM )
>> {
>>     int64_t v;
>>     uint32 res;
>>     int old_exc_flags = get_float_exception_flags(status);
>>
>>     v = float64_to_uint64(a STATUS_VAR);
>>     if (v > 0xffffffff) {
>>         res = 0xffffffff;
>>     } else {
>>         return v;
>>     }
>>     set_float_exception_flags(old_exc_flags);
>>     float_raise(float_flag_invalid STATUS_VAR);
>>     return res;
>> }
>>
>> thanks
>> -- PMM
>>
>
> This seems to assume that the only case where flags could be set in
> float64_to_uint32 is the case where a large result is returned. Is
> this really the case?

No, all it's assuming is that if we have the out-of-range
case then the only flag that should be set is Invalid.
In the "result is correct" case we return v and don't
modify the flags from what float64_to_uint64() has set.

Do you think there are flags that should be allowed
to be set by the conversion operation even if it is signaling
Invalid because of out of range input? IEEE754-2008 section 7.1
says "An invocation of [any operation except directly modifying
the flags] might raise at most two status flags, overflow
with inexact or underflow with inexact". That is, any operation
might (1) raise no flags (2) raise just one flag (3) raise
Overflow+Inexact (4) raise Underflow+Inexact.
[QEMU/softfloat don't suport alternate exception handling
so we can ignore the standard's verbiage about that.]

There is also the softfloat-specific float_flag_input_denormal,
but I think that is also fine because it will only be set by the
operation if we're squashing input denormals to zero, in
which case the float-to-int conversion must always successfully
return zero (because we squashed the input to plus or minus
zero).

This is a bit complicated though, so maybe I missed a case?

thanks
-- PMM
Tom Musta - Dec. 18, 2013, 6:23 p.m.
On 12/18/2013 12:03 PM, Peter Maydell wrote:
> On 18 December 2013 17:32, Tom Musta <tommusta@gmail.com> wrote:
>> On 12/17/2013 11:45 AM, Peter Maydell wrote:
>>
>> This seems to assume that the only case where flags could be set in
>> float64_to_uint32 is the case where a large result is returned. Is
>> this really the case?
> 
> No, all it's assuming is that if we have the out-of-range
> case then the only flag that should be set is Invalid.
> In the "result is correct" case we return v and don't
> modify the flags from what float64_to_uint64() has set.
> 
> Do you think there are flags that should be allowed
> to be set by the conversion operation even if it is signaling
> Invalid because of out of range input? IEEE754-2008 section 7.1
> says "An invocation of [any operation except directly modifying
> the flags] might raise at most two status flags, overflow
> with inexact or underflow with inexact". That is, any operation
> might (1) raise no flags (2) raise just one flag (3) raise
> Overflow+Inexact (4) raise Underflow+Inexact.
> [QEMU/softfloat don't suport alternate exception handling
> so we can ignore the standard's verbiage about that.]
> 
> There is also the softfloat-specific float_flag_input_denormal,
> but I think that is also fine because it will only be set by the
> operation if we're squashing input denormals to zero, in
> which case the float-to-int conversion must always successfully
> return zero (because we squashed the input to plus or minus
> zero).
> 
> This is a bit complicated though, so maybe I missed a case?
> 
> thanks
> -- PMM
> 

I'm sorry Peter ... I misread your patch.  I see what you are doing
now.

I'll use your pattern, retest and resubmit.

Patch

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 1003e59..6a8b6f5 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -6578,15 +6578,13 @@  uint_fast16_t float32_to_uint16_round_to_zero(float32 a STATUS_PARAM)
 
 uint32 float64_to_uint32( float64 a STATUS_PARAM )
 {
-    int64_t v;
+    uint64_t v;
     uint32 res;
 
-    v = float64_to_int64(a STATUS_VAR);
-    if (v < 0) {
-        res = 0;
-        float_raise( float_flag_invalid STATUS_VAR);
-    } else if (v > 0xffffffff) {
+    v = float64_to_uint64(a STATUS_VAR);
+    if (v > 0xffffffff) {
         res = 0xffffffff;
+        STATUS(float_exception_flags) &= ~float_flag_inexact;
         float_raise( float_flag_invalid STATUS_VAR);
     } else {
         res = v;