| Message ID | 1292500278-26215-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 16, 2010 at 11:51:17AM +0000, Peter Maydell wrote: > IEEE754 doesn't specify precisely what NaN should be returned as > the result of an operation on two input NaNs. This is therefore > target-specific. Abstract out the code in propagateFloat*NaN() > which was implementing the x87 propagation rules, so that it > can be easily replaced on a per-target basis. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > fpu/softfloat-specialize.h | 160 +++++++++++++++++++++++++++---------------- > 1 files changed, 100 insertions(+), 60 deletions(-) > > diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h > index 8e6aceb..3015480 100644 > --- a/fpu/softfloat-specialize.h > +++ b/fpu/softfloat-specialize.h > @@ -134,6 +134,52 @@ static float32 commonNaNToFloat32( commonNaNT a ) > } > > /*---------------------------------------------------------------------------- > +| Select which NaN to propagate for a two-input operation. > +| IEEE754 doesn't specify all the details of this, so the > +| algorithm is target-specific. > +| The routine is passed various bits of information about the > +| two NaNs and should return 0 to select NaN a and 1 for NaN b. > +| Note that signalling NaNs are always squashed to quiet NaNs > +| by the caller, by flipping the SNaN bit before returning them. > +| > +| aIsLargerSignificand is only valid if both a and b are NaNs > +| of some kind, and is true if a has the larger significand, > +| or if both a and b have the same significand but a is > +| positive but b is negative. It is only needed for the x87 > +| tie-break rule. > +*----------------------------------------------------------------------------*/ > + > +static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN, > + flag aIsLargerSignificand) > +{ > + /* This implements x87 NaN propagation rules: > + * SNaN + QNaN => return the QNaN > + * two SNaNs => return the one with the larger significand, silenced > + * two QNaNs => return the one with the larger significand > + * SNaN and a non-NaN => return the SNaN, silenced > + * QNaN and a non-NaN => return the QNaN > + * > + * If we get down to comparing significands and they are the same, > + * return the NaN with the positive sign bit (if any). > + */ > + if (aIsSNaN) { > + if (bIsSNaN) { > + return aIsLargerSignificand ? 0 : 1; > + } > + return bIsQNaN ? 1 : 0; > + } > + else if (aIsQNaN) { > + if (bIsSNaN || !bIsQNaN) > + return 0; > + else { > + return aIsLargerSignificand ? 0 : 1; > + } > + } else { > + return 1; > + } > +} I am basically find with the idea. I have tried to implement that for MIPS, but it seems your current implementation doesn't allow correct propagation for MIPS: if one of the two operand are a sNaN, the result should be a *default* qNaN. It seems that we should pass the operands to the pickNaN() function and return the result instead of a flag. That means having one pickNaN function per float type, but that can probably be handled by macros or by having a common function for targets on which its possible to do so. Note however that the current implementation provides the correct result, as the result is converted in op_helper.c: if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID) wt2 = FLOAT_QNAN32; However if we finally implement correct NaN propagation in the softfloat routines, it's better to have everything implemented there.
On 2 January 2011 13:23, Aurelien Jarno <aurelien@aurel32.net> wrote: > On Thu, Dec 16, 2010 at 11:51:17AM +0000, Peter Maydell wrote: >> IEEE754 doesn't specify precisely what NaN should be returned as >> the result of an operation on two input NaNs. This is therefore >> target-specific. Abstract out the code in propagateFloat*NaN() >> which was implementing the x87 propagation rules, so that it >> can be easily replaced on a per-target basis. > I am basically find with the idea. I have tried to implement that for > MIPS, but it seems your current implementation doesn't allow correct > propagation for MIPS: if one of the two operand are a sNaN, the result > should be a *default* qNaN. So if the input is a QNaN it's passed through but if it's an SNaN you get the default QNaN? I guess it makes sense since MIPS has SNAN_BIT_IS_ONE and you can't just silence a NaN by flipping the bit (because you might end up with a non-NaN if the final significand is all-zeroes). [...and the existing code that tries to do that is therefore wrong, presumably for both MIPS and HPPA.] Could we have a target-specific "silence this SNaN" function? Then the top level functions could use those rather than doing their own bit-flipping, and I think that would do the right thing for MIPS (you'd implement silence-NaN as "return the default QNaN", and you implement pickNaN() to return the SNaN.) > It seems that we should pass the operands to the pickNaN() function and > return the result instead of a flag. That means having one pickNaN > function per float type, but that can probably be handled by macros or > by having a common function for targets on which its possible to do so. As you might have guessed I was definitely trying to avoid having to actually pass the operands to pickNaN()... > Note however that the current implementation provides the correct > result, as the result is converted in op_helper.c: > > if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID) > wt2 = FLOAT_QNAN32; ...incidentally, I don't know MIPS but this code looks a bit suspect to me: set_float_exception_flags(0, &env->active_fpu.fp_status); \ wt2 = float32_ ## name (fst0, fst1, &env->active_fpu.fp_status); \ update_fcr31(); \ if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID) \ wt2 = FLOAT_QNAN32; \ Isn't it clearing the FP exception flags for each instruction when they ought to be cumulative? -- PMM
On Sun, Jan 02, 2011 at 02:04:11PM +0000, Peter Maydell wrote: > On 2 January 2011 13:23, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On Thu, Dec 16, 2010 at 11:51:17AM +0000, Peter Maydell wrote: > >> IEEE754 doesn't specify precisely what NaN should be returned as > >> the result of an operation on two input NaNs. This is therefore > >> target-specific. Abstract out the code in propagateFloat*NaN() > >> which was implementing the x87 propagation rules, so that it > >> can be easily replaced on a per-target basis. > > > I am basically find with the idea. I have tried to implement that for > > MIPS, but it seems your current implementation doesn't allow correct > > propagation for MIPS: if one of the two operand are a sNaN, the result > > should be a *default* qNaN. > > So if the input is a QNaN it's passed through but if it's an SNaN > you get the default QNaN? I guess it makes sense since MIPS Correct. > has SNAN_BIT_IS_ONE and you can't just silence a NaN by > flipping the bit (because you might end up with a non-NaN if > the final significand is all-zeroes). [...and the existing code that > tries to do that is therefore wrong, presumably for both MIPS > and HPPA.] This is also correct. We should probably change it to the default sNaN if the resulting significand is all-zeroes. > Could we have a target-specific "silence this SNaN" function? You mean a target-specific version of floatXX_maybe_silence_nan()? At least having a default version (for !SNAN_BIT_IS_ONE) and a version for MIPS, HPPA (which is btw not emulated), etc. > Then the top level functions could use those rather than doing > their own bit-flipping, and I think that would do the right thing > for MIPS (you'd implement silence-NaN as "return the default > QNaN", and you implement pickNaN() to return the SNaN.) It seems, it would be the good way to do it. I am going to do a quick hack to see if this solution can work and if it is the case (it seems to be), apply your patch. > > It seems that we should pass the operands to the pickNaN() function and > > return the result instead of a flag. That means having one pickNaN > > function per float type, but that can probably be handled by macros or > > by having a common function for targets on which its possible to do so. > > As you might have guessed I was definitely trying to avoid having > to actually pass the operands to pickNaN()... I agree with you that if it can be avoided, it's the best option. > > Note however that the current implementation provides the correct > > result, as the result is converted in op_helper.c: > > > > if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID) > > wt2 = FLOAT_QNAN32; > > ...incidentally, I don't know MIPS but this code looks a bit suspect to me: > set_float_exception_flags(0, &env->active_fpu.fp_status); \ > wt2 = float32_ ## name (fst0, fst1, &env->active_fpu.fp_status); \ > update_fcr31(); \ > if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID) \ > wt2 = FLOAT_QNAN32; \ > > Isn't it clearing the FP exception flags for each instruction when > they ought to be cumulative? > It only clears the softfloat ones, the real ones are kept in CPU register fcr31, which is updated by update_fcr31(). This allows to correctly emulate fcr31, as it contains a part that cumulates IEEE754 flags and can be reset by software, and a part that cumulates exception flags, which triggers exceptions if enabled, and which can be reset independently.
On 2 January 2011 15:05, Aurelien Jarno <aurelien@aurel32.net> wrote: > On Sun, Jan 02, 2011 at 02:04:11PM +0000, Peter Maydell wrote: >> Could we have a target-specific "silence this SNaN" function? > > You mean a target-specific version of floatXX_maybe_silence_nan()? Actually what I had in mind was a target-specific floatXX_silence_snan() which would be called from the propagateFloatXX_NaN() and floatXX_maybe_silence_nan() functions. But now you mention it, just allowing the target to override floatXX_maybe_silence_nan() (and calling it in propagateFloatXX_NaN()) looks like the better thing. > At least having a default version (for !SNAN_BIT_IS_ONE) and a version > for MIPS, HPPA (which is btw not emulated), etc. Yes. (Incidentally, HPPA says the rule for "silence this NaN" is: * set the first bit of the significand to 0 * set the second bit of the significand to 1 (implementations can do this (a) always or (b) only if all the other bits of the significand are 0) Of course we don't need to actually implement this since as you say we don't have an HPPA target, but it does suggest that "silencing rules are target-specific" is the right approach.) >> Then the top level functions could use those rather than doing >> their own bit-flipping, and I think that would do the right thing >> for MIPS (you'd implement silence-NaN as "return the default >> QNaN", and you implement pickNaN() to return the SNaN.) > > It seems, it would be the good way to do it. I am going to do a quick > hack to see if this solution can work and if it is the case (it seems > to be), apply your patch. Cool. >> Isn't it clearing the FP exception flags for each instruction when >> they ought to be cumulative? > It only clears the softfloat ones, the real ones are kept in CPU > register fcr31, which is updated by update_fcr31(). This allows to > correctly emulate fcr31, as it contains a part that cumulates IEEE754 > flags and can be reset by software, and a part that cumulates exception > flags, which triggers exceptions if enabled, and which can be reset > independently. Ah yes, I hadn't spotted that we were calling UPDATE_FP_FLAGS rather than SET_FP_FLAGS in update_fcr31(). Sorry for the false alarm. -- PMM
On Sun, Jan 02, 2011 at 03:35:30PM +0000, Peter Maydell wrote: > On 2 January 2011 15:05, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On Sun, Jan 02, 2011 at 02:04:11PM +0000, Peter Maydell wrote: > >> Could we have a target-specific "silence this SNaN" function? > > > > You mean a target-specific version of floatXX_maybe_silence_nan()? > > Actually what I had in mind was a target-specific floatXX_silence_snan() > which would be called from the propagateFloatXX_NaN() > and floatXX_maybe_silence_nan() functions. But now you mention > it, just allowing the target to override floatXX_maybe_silence_nan() > (and calling it in propagateFloatXX_NaN()) looks like the better thing. > > > At least having a default version (for !SNAN_BIT_IS_ONE) and a version > > for MIPS, HPPA (which is btw not emulated), etc. > > Yes. (Incidentally, HPPA says the rule for "silence this NaN" is: > * set the first bit of the significand to 0 > * set the second bit of the significand to 1 (implementations can do > this (a) always or (b) only if all the other bits of the significand are 0) > Of course we don't need to actually implement this since > as you say we don't have an HPPA target, but it does suggest > that "silencing rules are target-specific" is the right approach.) > > >> Then the top level functions could use those rather than doing > >> their own bit-flipping, and I think that would do the right thing > >> for MIPS (you'd implement silence-NaN as "return the default > >> QNaN", and you implement pickNaN() to return the SNaN.) > > > > It seems, it would be the good way to do it. I am going to do a quick > > hack to see if this solution can work and if it is the case (it seems > > to be), apply your patch. > > Cool. > I confirm this solution works. I have applied your patches, I'll send a patch series to implement correct propagation on MIPS later this week.
diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h index 8e6aceb..3015480 100644 --- a/fpu/softfloat-specialize.h +++ b/fpu/softfloat-specialize.h @@ -134,6 +134,52 @@ static float32 commonNaNToFloat32( commonNaNT a ) } /*---------------------------------------------------------------------------- +| Select which NaN to propagate for a two-input operation. +| IEEE754 doesn't specify all the details of this, so the +| algorithm is target-specific. +| The routine is passed various bits of information about the +| two NaNs and should return 0 to select NaN a and 1 for NaN b. +| Note that signalling NaNs are always squashed to quiet NaNs +| by the caller, by flipping the SNaN bit before returning them. +| +| aIsLargerSignificand is only valid if both a and b are NaNs +| of some kind, and is true if a has the larger significand, +| or if both a and b have the same significand but a is +| positive but b is negative. It is only needed for the x87 +| tie-break rule. +*----------------------------------------------------------------------------*/ + +static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN, + flag aIsLargerSignificand) +{ + /* This implements x87 NaN propagation rules: + * SNaN + QNaN => return the QNaN + * two SNaNs => return the one with the larger significand, silenced + * two QNaNs => return the one with the larger significand + * SNaN and a non-NaN => return the SNaN, silenced + * QNaN and a non-NaN => return the QNaN + * + * If we get down to comparing significands and they are the same, + * return the NaN with the positive sign bit (if any). + */ + if (aIsSNaN) { + if (bIsSNaN) { + return aIsLargerSignificand ? 0 : 1; + } + return bIsQNaN ? 1 : 0; + } + else if (aIsQNaN) { + if (bIsSNaN || !bIsQNaN) + return 0; + else { + return aIsLargerSignificand ? 0 : 1; + } + } else { + return 1; + } +} + +/*---------------------------------------------------------------------------- | Takes two single-precision floating-point values `a' and `b', one of which | is a NaN, and returns the appropriate NaN result. If either `a' or `b' is a | signaling NaN, the invalid exception is raised. @@ -141,7 +187,7 @@ static float32 commonNaNToFloat32( commonNaNT a ) static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM) { - flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN; + flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, aIsLargerSignificand; bits32 av, bv, res; if ( STATUS(default_nan_mode) ) @@ -161,26 +207,22 @@ static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM) bv |= 0x00400000; #endif if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR); - if ( aIsSignalingNaN ) { - if ( bIsSignalingNaN ) goto returnLargerSignificand; - res = bIsNaN ? bv : av; - } - else if ( aIsNaN ) { - if ( bIsSignalingNaN || ! bIsNaN ) - res = av; - else { - returnLargerSignificand: - if ( (bits32) ( av<<1 ) < (bits32) ( bv<<1 ) ) - res = bv; - else if ( (bits32) ( bv<<1 ) < (bits32) ( av<<1 ) ) - res = av; - else - res = ( av < bv ) ? av : bv; - } + + if ((bits32)(av<<1) < (bits32)(bv<<1)) { + aIsLargerSignificand = 0; + } else if ((bits32)(bv<<1) < (bits32)(av<<1)) { + aIsLargerSignificand = 1; + } else { + aIsLargerSignificand = (av < bv) ? 1 : 0; } - else { + + if (pickNaN(aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, + aIsLargerSignificand)) { res = bv; + } else { + res = av; } + return make_float32(res); } @@ -276,7 +318,7 @@ static float64 commonNaNToFloat64( commonNaNT a ) static float64 propagateFloat64NaN( float64 a, float64 b STATUS_PARAM) { - flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN; + flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, aIsLargerSignificand; bits64 av, bv, res; if ( STATUS(default_nan_mode) ) @@ -296,26 +338,22 @@ static float64 propagateFloat64NaN( float64 a, float64 b STATUS_PARAM) bv |= LIT64( 0x0008000000000000 ); #endif if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR); - if ( aIsSignalingNaN ) { - if ( bIsSignalingNaN ) goto returnLargerSignificand; - res = bIsNaN ? bv : av; - } - else if ( aIsNaN ) { - if ( bIsSignalingNaN || ! bIsNaN ) - res = av; - else { - returnLargerSignificand: - if ( (bits64) ( av<<1 ) < (bits64) ( bv<<1 ) ) - res = bv; - else if ( (bits64) ( bv<<1 ) < (bits64) ( av<<1 ) ) - res = av; - else - res = ( av < bv ) ? av : bv; - } + + if ((bits64)(av<<1) < (bits64)(bv<<1)) { + aIsLargerSignificand = 0; + } else if ((bits64)(bv<<1) < (bits64)(av<<1)) { + aIsLargerSignificand = 1; + } else { + aIsLargerSignificand = (av < bv) ? 1 : 0; } - else { + + if (pickNaN(aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, + aIsLargerSignificand)) { res = bv; + } else { + res = av; } + return make_float64(res); } @@ -416,7 +454,7 @@ static floatx80 commonNaNToFloatx80( commonNaNT a ) static floatx80 propagateFloatx80NaN( floatx80 a, floatx80 b STATUS_PARAM) { - flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN; + flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, aIsLargerSignificand; if ( STATUS(default_nan_mode) ) { a.low = floatx80_default_nan_low; @@ -436,19 +474,20 @@ static floatx80 propagateFloatx80NaN( floatx80 a, floatx80 b STATUS_PARAM) b.low |= LIT64( 0xC000000000000000 ); #endif if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR); - if ( aIsSignalingNaN ) { - if ( bIsSignalingNaN ) goto returnLargerSignificand; - return bIsNaN ? b : a; - } - else if ( aIsNaN ) { - if ( bIsSignalingNaN || ! bIsNaN ) return a; - returnLargerSignificand: - if ( a.low < b.low ) return b; - if ( b.low < a.low ) return a; - return ( a.high < b.high ) ? a : b; + + if (a.low < b.low) { + aIsLargerSignificand = 0; + } else if (b.low < a.low) { + aIsLargerSignificand = 1; + } else { + aIsLargerSignificand = (a.high < b.high) ? 1 : 0; } - else { + + if (pickNaN(aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, + aIsLargerSignificand)) { return b; + } else { + return a; } } @@ -542,7 +581,7 @@ static float128 commonNaNToFloat128( commonNaNT a ) static float128 propagateFloat128NaN( float128 a, float128 b STATUS_PARAM) { - flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN; + flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, aIsLargerSignificand; if ( STATUS(default_nan_mode) ) { a.low = float128_default_nan_low; @@ -562,19 +601,20 @@ static float128 propagateFloat128NaN( float128 a, float128 b STATUS_PARAM) b.high |= LIT64( 0x0000800000000000 ); #endif if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR); - if ( aIsSignalingNaN ) { - if ( bIsSignalingNaN ) goto returnLargerSignificand; - return bIsNaN ? b : a; - } - else if ( aIsNaN ) { - if ( bIsSignalingNaN || ! bIsNaN ) return a; - returnLargerSignificand: - if ( lt128( a.high<<1, a.low, b.high<<1, b.low ) ) return b; - if ( lt128( b.high<<1, b.low, a.high<<1, a.low ) ) return a; - return ( a.high < b.high ) ? a : b; + + if (lt128(a.high<<1, a.low, b.high<<1, b.low)) { + aIsLargerSignificand = 0; + } else if (lt128(b.high<<1, b.low, a.high<<1, a.low)) { + aIsLargerSignificand = 1; + } else { + aIsLargerSignificand = (a.high < b.high) ? 1 : 0; } - else { + + if (pickNaN(aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, + aIsLargerSignificand)) { return b; + } else { + return a; } }
IEEE754 doesn't specify precisely what NaN should be returned as the result of an operation on two input NaNs. This is therefore target-specific. Abstract out the code in propagateFloat*NaN() which was implementing the x87 propagation rules, so that it can be easily replaced on a per-target basis. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- fpu/softfloat-specialize.h | 160 +++++++++++++++++++++++++++---------------- 1 files changed, 100 insertions(+), 60 deletions(-)