Patchwork [4/6] softfloat: use float{32, 64, x80, 128}_maybe_silence_nan()

login
register
mail settings
Submitter Aurelien Jarno
Date Jan. 3, 2011, 2:34 p.m.
Message ID <1294065273-30274-5-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/77277/
State New
Headers show

Comments

Aurelien Jarno - Jan. 3, 2011, 2:34 p.m.
Use float{32,64,x80,128}_maybe_silence_nan() instead of toggling the
sNaN bit manually. This allow per target implementation of sNaN to qNaN
conversion.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-specialize.h |   55 +++++++++++++------------------------------
 1 files changed, 17 insertions(+), 38 deletions(-)
Peter Maydell - Jan. 3, 2011, 5:34 p.m.
On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
> Use float{32,64,x80,128}_maybe_silence_nan() instead of toggling the
> sNaN bit manually. This allow per target implementation of sNaN to qNaN
> conversion.

> @@ -237,15 +237,11 @@ static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM)
>     aIsSignalingNaN = float32_is_signaling_nan( a );
>     bIsNaN = float32_is_quiet_nan( b );
>     bIsSignalingNaN = float32_is_signaling_nan( b );
> +    a = float32_maybe_silence_nan(a);
> +    b = float32_maybe_silence_nan(b);
>     av = float32_val(a);
>     bv = float32_val(b);
> -#if SNAN_BIT_IS_ONE
> -    av &= ~0x00400000;
> -    bv &= ~0x00400000;
> -#else
> -    av |= 0x00400000;
> -    bv |= 0x00400000;
> -#endif
> +
>     if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR);
>
>     if ((bits32)(av<<1) < (bits32)(bv<<1)) {

The place you've put the maybe_silence_nan() calls means that
the calculation of aIsLargerSignificand is done based on the silenced
values rather than on the input values, which I think is wrong (it
is inconsistent with the *isSignalingNaN and *isNaN flags).

It doesn't make any difference in practice, because the only thing
that uses it is the x87 rules, and they only look at aIsLargerSignificand
if both a and b are the same type of NaN, in which case by x87
silencing rules the comparison gives the same result whether it's
before or after silencing. But I think conceptually it would be better
to call float*_maybe_silence_nan() after the calculation of
aIsLargerSignificand.

Unrelated trivial cleanup which I've just noticed and which you might
want to throw into v2 of this patchset:
>     bIsNaN = float32_is_quiet_nan( b );

...these variables would be better named as aIsQNan, bIsQNaN in
these propagate* functions.

-- PMM
Aurelien Jarno - Jan. 3, 2011, 10:44 p.m.
On Mon, Jan 03, 2011 at 05:34:17PM +0000, Peter Maydell wrote:
> On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > Use float{32,64,x80,128}_maybe_silence_nan() instead of toggling the
> > sNaN bit manually. This allow per target implementation of sNaN to qNaN
> > conversion.
> 
> > @@ -237,15 +237,11 @@ static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM)
> >     aIsSignalingNaN = float32_is_signaling_nan( a );
> >     bIsNaN = float32_is_quiet_nan( b );
> >     bIsSignalingNaN = float32_is_signaling_nan( b );
> > +    a = float32_maybe_silence_nan(a);
> > +    b = float32_maybe_silence_nan(b);
> >     av = float32_val(a);
> >     bv = float32_val(b);
> > -#if SNAN_BIT_IS_ONE
> > -    av &= ~0x00400000;
> > -    bv &= ~0x00400000;
> > -#else
> > -    av |= 0x00400000;
> > -    bv |= 0x00400000;
> > -#endif
> > +
> >     if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR);
> >
> >     if ((bits32)(av<<1) < (bits32)(bv<<1)) {
> 
> The place you've put the maybe_silence_nan() calls means that
> the calculation of aIsLargerSignificand is done based on the silenced
> values rather than on the input values, which I think is wrong (it
> is inconsistent with the *isSignalingNaN and *isNaN flags).
> 
> It doesn't make any difference in practice, because the only thing
> that uses it is the x87 rules, and they only look at aIsLargerSignificand
> if both a and b are the same type of NaN, in which case by x87
> silencing rules the comparison gives the same result whether it's
> before or after silencing. But I think conceptually it would be better
> to call float*_maybe_silence_nan() after the calculation of
> aIsLargerSignificand.

I agree it should not make any change, but that it is conceptually
wrong. I'll fix that in v2.

> Unrelated trivial cleanup which I've just noticed and which you might
> want to throw into v2 of this patchset:
> >     bIsNaN = float32_is_quiet_nan( b );
> 
> ...these variables would be better named as aIsQNan, bIsQNaN in
> these propagate* functions.
> 

Probably to "aIsQuietNaN" to match the "aIsSignalingNaN" name.

I'll send the v2 tomorrow.

Patch

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index 49e3cc2..4deb165 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -228,7 +228,7 @@  static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
 static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM)
 {
     flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, aIsLargerSignificand;
-    bits32 av, bv, res;
+    bits32 av, bv;
 
     if ( STATUS(default_nan_mode) )
         return float32_default_nan;
@@ -237,15 +237,11 @@  static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM)
     aIsSignalingNaN = float32_is_signaling_nan( a );
     bIsNaN = float32_is_quiet_nan( b );
     bIsSignalingNaN = float32_is_signaling_nan( b );
+    a = float32_maybe_silence_nan(a);
+    b = float32_maybe_silence_nan(b);
     av = float32_val(a);
     bv = float32_val(b);
-#if SNAN_BIT_IS_ONE
-    av &= ~0x00400000;
-    bv &= ~0x00400000;
-#else
-    av |= 0x00400000;
-    bv |= 0x00400000;
-#endif
+
     if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR);
 
     if ((bits32)(av<<1) < (bits32)(bv<<1)) {
@@ -258,12 +254,10 @@  static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM)
 
     if (pickNaN(aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN,
                 aIsLargerSignificand)) {
-        res = bv;
+        return b;
     } else {
-        res = av;
+        return a;
     }
-
-    return make_float32(res);
 }
 
 /*----------------------------------------------------------------------------
@@ -376,7 +370,7 @@  static float64 commonNaNToFloat64( commonNaNT a )
 static float64 propagateFloat64NaN( float64 a, float64 b STATUS_PARAM)
 {
     flag aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN, aIsLargerSignificand;
-    bits64 av, bv, res;
+    bits64 av, bv;
 
     if ( STATUS(default_nan_mode) )
         return float64_default_nan;
@@ -385,15 +379,10 @@  static float64 propagateFloat64NaN( float64 a, float64 b STATUS_PARAM)
     aIsSignalingNaN = float64_is_signaling_nan( a );
     bIsNaN = float64_is_quiet_nan( b );
     bIsSignalingNaN = float64_is_signaling_nan( b );
+    a = float64_maybe_silence_nan(a);
+    b = float64_maybe_silence_nan(b);
     av = float64_val(a);
     bv = float64_val(b);
-#if SNAN_BIT_IS_ONE
-    av &= ~LIT64( 0x0008000000000000 );
-    bv &= ~LIT64( 0x0008000000000000 );
-#else
-    av |= LIT64( 0x0008000000000000 );
-    bv |= LIT64( 0x0008000000000000 );
-#endif
     if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR);
 
     if ((bits64)(av<<1) < (bits64)(bv<<1)) {
@@ -406,12 +395,10 @@  static float64 propagateFloat64NaN( float64 a, float64 b STATUS_PARAM)
 
     if (pickNaN(aIsNaN, aIsSignalingNaN, bIsNaN, bIsSignalingNaN,
                 aIsLargerSignificand)) {
-        res = bv;
+        return b;
     } else {
-        res = av;
+        return a;
     }
-
-    return make_float64(res);
 }
 
 #ifdef FLOATX80
@@ -542,13 +529,9 @@  static floatx80 propagateFloatx80NaN( floatx80 a, floatx80 b STATUS_PARAM)
     aIsSignalingNaN = floatx80_is_signaling_nan( a );
     bIsNaN = floatx80_is_quiet_nan( b );
     bIsSignalingNaN = floatx80_is_signaling_nan( b );
-#if SNAN_BIT_IS_ONE
-    a.low &= ~LIT64( 0xC000000000000000 );
-    b.low &= ~LIT64( 0xC000000000000000 );
-#else
-    a.low |= LIT64( 0xC000000000000000 );
-    b.low |= LIT64( 0xC000000000000000 );
-#endif
+    a = floatx80_maybe_silence_nan(a);
+    a = floatx80_maybe_silence_nan(b);
+
     if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR);
 
     if (a.low < b.low) {
@@ -688,13 +671,9 @@  static float128 propagateFloat128NaN( float128 a, float128 b STATUS_PARAM)
     aIsSignalingNaN = float128_is_signaling_nan( a );
     bIsNaN = float128_is_quiet_nan( b );
     bIsSignalingNaN = float128_is_signaling_nan( b );
-#if SNAN_BIT_IS_ONE
-    a.high &= ~LIT64( 0x0000800000000000 );
-    b.high &= ~LIT64( 0x0000800000000000 );
-#else
-    a.high |= LIT64( 0x0000800000000000 );
-    b.high |= LIT64( 0x0000800000000000 );
-#endif
+    a = float128_maybe_silence_nan(a);
+    b = float128_maybe_silence_nan(b);
+
     if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise( float_flag_invalid STATUS_VAR);
 
     if (lt128(a.high<<1, a.low, b.high<<1, b.low)) {