Patchwork [1/2] softfloat: abstract out target-specific NaN propagation rules

login
register
mail settings
Submitter Peter Maydell
Date Dec. 16, 2010, 11:51 a.m.
Message ID <1292500278-26215-2-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/75742/
State New
Headers show

Comments

Peter Maydell - Dec. 16, 2010, 11:51 a.m.
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(-)
Aurelien Jarno - Jan. 2, 2011, 1:23 p.m.
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.
Peter Maydell - Jan. 2, 2011, 2:04 p.m.
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
Aurelien Jarno - Jan. 2, 2011, 3:05 p.m.
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.
Peter Maydell - Jan. 2, 2011, 3:35 p.m.
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
Aurelien Jarno - Jan. 2, 2011, 11 p.m.
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.

Patch

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;
     }
 }