| Message ID | 1386789398-5239-12-git-send-email-tommusta@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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; }
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
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.
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;
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(-)