Softfloat: Add support to softfloat to return floatxx_default_nan when, the corresponding target status flag is set.

PATCH In review Christophe Lyon 3 participants 9 messages Feb 7, 2011 – Feb 21, 2011

Share & tools

Christophe Lyon

author

2011-02-07 17:00 UTC #0 lore raw

PATCH v3 1/1 Softfloat: Add support to softfloat to return floatxx_default_nan when, the corresponding target status flag is set.

Some CPUs have a status flag imposing to return floatxx_default_nan whatever the input value, when converting from one FP format to another. Implement this, using the already existing default_nan_mode status flag (currently used on ARM and SH4 at the moment).

Signed-off-by Christophe Lyon <[email protected]>

---
 fpu/softfloat-specialize.h |   60 +++++++++++++++++++++++++++++++++++++++++---
 fpu/softfloat.c            |   30 +++++++++------------
 2 files changed, 69 insertions(+), 21 deletions(-)

fpu/softfloat-specialize.h +56 -4

index 11521ce..3f7995c 100644
@@ -144,9 +144,14 @@ static commonNaNT float32ToCommonNaN( float32 a STATUS_PARAM )
 | precision floating-point format.
 *----------------------------------------------------------------------------*/
 
-static float32 commonNaNToFloat32( commonNaNT a )
+static float32 commonNaNToFloat32( commonNaNT a STATUS_PARAM)
 {
     bits32 mantissa = a.high>>41;
+
+    if ( STATUS(default_nan_mode) ) {
+        return float32_default_nan;
+    }
+
     if ( mantissa )
         return make_float32(
             ( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) );

@@ -398,10 +403,14 @@ static commonNaNT float64ToCommonNaN( float64 a STATUS_PARAM)
 | precision floating-point format.
 *----------------------------------------------------------------------------*/
 
-static float64 commonNaNToFloat64( commonNaNT a )
+static float64 commonNaNToFloat64( commonNaNT a STATUS_PARAM)
 {
     bits64 mantissa = a.high>>12;
 
+    if ( STATUS(default_nan_mode) ) {
+        return float64_default_nan;
+    }
+
     if ( mantissa )
         return make_float64(
               ( ( (bits64) a.sign )<<63 )

@@ -451,6 +460,37 @@ static float64 propagateFloat64NaN( float64 a, float64 b STATUS_PARAM)
     }
 }
 
+/*----------------------------------------------------------------------------
+| The pattern for a default generated haft-precision NaN.
+*----------------------------------------------------------------------------*/
+#if defined(TARGET_ARM)
+#define float16_default_nan 0x7E00
+#elif SNAN_BIT_IS_ONE
+#define float16_default_nan 0x7DFF
+#else
+#define float16_default_nan 0xFE00
+#endif
+
+/*----------------------------------------------------------------------------
+| Returns the result of converting the canonical NaN `a' to the half-
+| precision floating-point format.
+*----------------------------------------------------------------------------*/
+
+static bits16 commonNaNToFloat16( commonNaNT a STATUS_PARAM)
+{
+    bits16 mantissa = a.high>>57;
+
+    if ( STATUS(default_nan_mode) ) {
+        return float16_default_nan;
+    }
+
+    if ( mantissa ) {
+        return ( ( ( (bits16) a.sign) << 15) | (0x1F << 10) | mantissa );
+    } else {
+        return float16_default_nan;
+    }
+}
+
 #ifdef FLOATX80
 
 /*----------------------------------------------------------------------------

@@ -551,10 +591,16 @@ static commonNaNT floatx80ToCommonNaN( floatx80 a STATUS_PARAM)
 | double-precision floating-point format.
 *----------------------------------------------------------------------------*/
 
-static floatx80 commonNaNToFloatx80( commonNaNT a )
+static floatx80 commonNaNToFloatx80( commonNaNT a STATUS_PARAM)
 {
     floatx80 z;
 
+    if ( STATUS(default_nan_mode) ) {
+        z.low = floatx80_default_nan_low;
+        z.high = floatx80_default_nan_high;
+        return z;
+    }
+
     if (a.high)
         z.low = a.high;
     else

@@ -699,10 +745,16 @@ static commonNaNT float128ToCommonNaN( float128 a STATUS_PARAM)
 | precision floating-point format.
 *----------------------------------------------------------------------------*/
 
-static float128 commonNaNToFloat128( commonNaNT a )
+static float128 commonNaNToFloat128( commonNaNT a STATUS_PARAM)
 {
     float128 z;
 
+    if ( STATUS(default_nan_mode) ) {
+        z.low = float128_default_nan_low;
+        z.high = float128_default_nan_high;
+        return z;
+    }
+
     shift128Right( a.high, a.low, 16, &z.high, &z.low );
     z.high |= ( ( (bits64) a.sign )<<63 ) | LIT64( 0x7FFF000000000000 );
     return z;

fpu/softfloat.c +13 ~~-17~~

index 17842f4..abf14f1 100644
@@ -1534,7 +1534,7 @@ float64 float32_to_float64( float32 a STATUS_PARAM )
     aExp = extractFloat32Exp( a );
     aSign = extractFloat32Sign( a );
     if ( aExp == 0xFF ) {
-        if ( aSig ) return commonNaNToFloat64( float32ToCommonNaN( a STATUS_VAR ));
+        if ( aSig ) return commonNaNToFloat64( float32ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         return packFloat64( aSign, 0x7FF, 0 );
     }
     if ( aExp == 0 ) {

@@ -1566,7 +1566,7 @@ floatx80 float32_to_floatx80( float32 a STATUS_PARAM )
     aExp = extractFloat32Exp( a );
     aSign = extractFloat32Sign( a );
     if ( aExp == 0xFF ) {
-        if ( aSig ) return commonNaNToFloatx80( float32ToCommonNaN( a STATUS_VAR ) );
+        if ( aSig ) return commonNaNToFloatx80( float32ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         return packFloatx80( aSign, 0x7FFF, LIT64( 0x8000000000000000 ) );
     }
     if ( aExp == 0 ) {

@@ -1600,7 +1600,7 @@ float128 float32_to_float128( float32 a STATUS_PARAM )
     aExp = extractFloat32Exp( a );
     aSign = extractFloat32Sign( a );
     if ( aExp == 0xFF ) {
-        if ( aSig ) return commonNaNToFloat128( float32ToCommonNaN( a STATUS_VAR ) );
+        if ( aSig ) return commonNaNToFloat128( float32ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         return packFloat128( aSign, 0x7FFF, 0, 0 );
     }
     if ( aExp == 0 ) {

@@ -2689,7 +2689,7 @@ float32 float64_to_float32( float64 a STATUS_PARAM )
     aExp = extractFloat64Exp( a );
     aSign = extractFloat64Sign( a );
     if ( aExp == 0x7FF ) {
-        if ( aSig ) return commonNaNToFloat32( float64ToCommonNaN( a STATUS_VAR ) );
+        if ( aSig ) return commonNaNToFloat32( float64ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         return packFloat32( aSign, 0xFF, 0 );
     }
     shift64RightJamming( aSig, 22, &aSig );

@@ -2767,11 +2767,7 @@ bits16 float32_to_float16( float32 a, flag ieee STATUS_PARAM)
     aExp = extractFloat32Exp( a );
     aSign = extractFloat32Sign( a );
     if ( aExp == 0xFF ) {
-        if (aSig) {
-            /* Make sure correct exceptions are raised.  */
-            float32ToCommonNaN(a STATUS_VAR);
-            aSig |= 0x00400000;
-        }
+        if ( aSig ) return commonNaNToFloat16( float32ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         return packFloat16(aSign, 0x1f, aSig >> 13);
     }
     if (aExp == 0 && aSign == 0) {

@@ -2861,7 +2857,7 @@ floatx80 float64_to_floatx80( float64 a STATUS_PARAM )
     aExp = extractFloat64Exp( a );
     aSign = extractFloat64Sign( a );
     if ( aExp == 0x7FF ) {
-        if ( aSig ) return commonNaNToFloatx80( float64ToCommonNaN( a STATUS_VAR ) );
+        if ( aSig ) return commonNaNToFloatx80( float64ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         return packFloatx80( aSign, 0x7FFF, LIT64( 0x8000000000000000 ) );
     }
     if ( aExp == 0 ) {

@@ -2896,7 +2892,7 @@ float128 float64_to_float128( float64 a STATUS_PARAM )
     aExp = extractFloat64Exp( a );
     aSign = extractFloat64Sign( a );
     if ( aExp == 0x7FF ) {
-        if ( aSig ) return commonNaNToFloat128( float64ToCommonNaN( a STATUS_VAR ) );
+        if ( aSig ) return commonNaNToFloat128( float64ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         return packFloat128( aSign, 0x7FFF, 0, 0 );
     }
     if ( aExp == 0 ) {

@@ -3843,7 +3839,7 @@ float32 floatx80_to_float32( floatx80 a STATUS_PARAM )
     aSign = extractFloatx80Sign( a );
     if ( aExp == 0x7FFF ) {
         if ( (bits64) ( aSig<<1 ) ) {
-            return commonNaNToFloat32( floatx80ToCommonNaN( a STATUS_VAR ) );
+            return commonNaNToFloat32( floatx80ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         }
         return packFloat32( aSign, 0xFF, 0 );
     }

@@ -3871,7 +3867,7 @@ float64 floatx80_to_float64( floatx80 a STATUS_PARAM )
     aSign = extractFloatx80Sign( a );
     if ( aExp == 0x7FFF ) {
         if ( (bits64) ( aSig<<1 ) ) {
-            return commonNaNToFloat64( floatx80ToCommonNaN( a STATUS_VAR ) );
+            return commonNaNToFloat64( floatx80ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         }
         return packFloat64( aSign, 0x7FF, 0 );
     }

@@ -3900,7 +3896,7 @@ float128 floatx80_to_float128( floatx80 a STATUS_PARAM )
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
     if ( ( aExp == 0x7FFF ) && (bits64) ( aSig<<1 ) ) {
-        return commonNaNToFloat128( floatx80ToCommonNaN( a STATUS_VAR ) );
+        return commonNaNToFloat128( floatx80ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
     }
     shift128Right( aSig<<1, 0, 16, &zSig0, &zSig1 );
     return packFloat128( aSign, aExp, zSig0, zSig1 );

@@ -4863,7 +4859,7 @@ float32 float128_to_float32( float128 a STATUS_PARAM )
     aSign = extractFloat128Sign( a );
     if ( aExp == 0x7FFF ) {
         if ( aSig0 | aSig1 ) {
-            return commonNaNToFloat32( float128ToCommonNaN( a STATUS_VAR ) );
+            return commonNaNToFloat32( float128ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         }
         return packFloat32( aSign, 0xFF, 0 );
     }

@@ -4897,7 +4893,7 @@ float64 float128_to_float64( float128 a STATUS_PARAM )
     aSign = extractFloat128Sign( a );
     if ( aExp == 0x7FFF ) {
         if ( aSig0 | aSig1 ) {
-            return commonNaNToFloat64( float128ToCommonNaN( a STATUS_VAR ) );
+            return commonNaNToFloat64( float128ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         }
         return packFloat64( aSign, 0x7FF, 0 );
     }

@@ -4932,7 +4928,7 @@ floatx80 float128_to_floatx80( float128 a STATUS_PARAM )
     aSign = extractFloat128Sign( a );
     if ( aExp == 0x7FFF ) {
         if ( aSig0 | aSig1 ) {
-            return commonNaNToFloatx80( float128ToCommonNaN( a STATUS_VAR ) );
+            return commonNaNToFloatx80( float128ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         }
         return packFloatx80( aSign, 0x7FFF, LIT64( 0x8000000000000000 ) );
     }

Signature · 1 line

1.7.2.3

Peter Maydell

2011-02-08 16:03 UTC #1 lore raw

Quoted 7 lines · Christophe Lyon

Some CPUs have a status flag imposing to return floatxx_default_nan whatever the input value, when converting from one FP format to another. Implement this, using the already existing default_nan_mode status flag (currently used on ARM and SH4 at the moment).
Signed-off-by Christophe Lyon <[email protected]>

Your subject is a bit long, which can make it wrap annoyingly in git logs. I'd suggest something like:

===begin===
softfloat: Honour default_nan_mode for float-to-float conversions

Honour the default_nan_mode flag when doing conversions between different floating point formats, as well as when returning a NaN from a two-operand floating point function. This corrects the behaviour of float<->double conversions on both ARM and SH4. ===endit===

fpu/softfloat-specialize.h static float64 propagateFloat64NaN( float64 a, float64 b STATUS_PARAM) · line 465

Quoted 3 lines

fpu/softfloat-specialize.h +3 -0

+/*----------------------------------------------------------------------------
+| The pattern for a default generated haft-precision NaN.
+*----------------------------------------------------------------------------*/

"half-precision".

fpu/softfloat.c bits16 float32_to_float16( float32 a, flag ieee STATUS_PARAM) · line 2773

Quoted 13 lines

fpu/softfloat.c +1 -5

@@ -2767,11 +2767,7 @@ bits16 float32_to_float16( float32 a, flag ieee STATUS_PARAM)
     aExp = extractFloat32Exp( a );
     aSign = extractFloat32Sign( a );
     if ( aExp == 0xFF ) {
-        if (aSig) {
-            /* Make sure correct exceptions are raised.  */
-            float32ToCommonNaN(a STATUS_VAR);
-            aSig |= 0x00400000;
-        }
+        if ( aSig ) return commonNaNToFloat16( float32ToCommonNaN( a STATUS_VAR ) STATUS_VAR );
         return packFloat16(aSign, 0x1f, aSig >> 13);
     }
     if (aExp == 0 && aSign == 0) {

This is float32_to_float16() but it looks like you've missed float16_to_float32() ?

Otherwise, looks good, and I can confirm that it fixes behaviour of VCVT.F32.F64 and VCVT.F64.F32 when the FPSCR.DN bit is set.

-- PMM

Christophe Lyon

author

2011-02-08 17:52 UTC #2 lore raw

Quoted 2 lines · Peter Maydell

Your subject is a bit long, which can make it wrap annoyingly in git logs. I'd suggest something like:

OK.

Quoted 2 lines

This is float32_to_float16() but it looks like you've missed float16_to_float32() ?

Well, actually I somewhat managed to persuade myself that it was not necessary to patch float16_to_float32(), but looking at it again it seems there are more bugs in that function:
- the ieee parameter shouldn't be used (my ARM ARM does not mention a different behaviour when AH bit is set)
- it calls float32ToCommonNaN(a) where 'a' is a float16 instead of a float32
- this means implementing float16ToCommonNaN, thus float16_is_signaling_nan()

Quoted 4 lines

Otherwise, looks good, and I can confirm that it fixes behaviour of VCVT.F32.F64 and VCVT.F64.F32 when the FPSCR.DN bit is set.

Thanks for the confirmation.

Christophe.

Peter Maydell

2011-02-08 18:53 UTC #3 lore raw

Quoted 6 lines · Christophe Lyon

Quoted 2 lines · Peter Maydell
This is float32_to_float16() but it looks like you've missed float16_to_float32() ?
Well, actually I somewhat managed to persuade myself that it was not necessary to patch float16_to_float32(), but looking at it again it seems there are more bugs in that function:
- the ieee parameter shouldn't be used (my ARM ARM does not mention a different behaviour when AH bit is set)

In the ARM ARM pseudocode, FPUnpack() of a 16 bit value looks at the FPSCR AH bit, so float16_to_float32() does need to depend on the ieee flag.

Quoted 1 line

- it calls float32ToCommonNaN(a) where 'a' is a float16 instead of a float32

It's only doing that to raise the exception, but you're right that it ought to do things properly...

Quoted 1 line

- this means implementing float16ToCommonNaN, thus float16_is_signaling_nan()

...like that, yes.

I'm just working through a set of bugs in float32_to_float16 and float16_to_float32 where they generally mishandle various things.

I have also just noticed this error in your patch:

+static bits16 commonNaNToFloat16( commonNaNT a STATUS_PARAM) +{
+ bits16 mantissa = a.high>>57;

That should be ">> 54". (We're shifting a mantissa which is in a 64 bit value down so that the top bits of it end up in the bottom 10 bits (corresponding to the size of mantissa on a float16); so we want to shift by 64-10 == 54.)

-- PMM

Peter Maydell

2011-02-08 20:07 UTC #4 lore raw

Quoted 4 lines · Peter Maydell

Quoted 1 line · Christophe Lyon
- this means implementing float16ToCommonNaN, thus float16_is_signaling_nan()
...like that, yes.

I have a slightly-tested implementation of these functions on my hard disk now; I'll try to finish testing them tomorrow (my testing is still falling over on some inputs to VCVTT/VCVTB).

(cc: Aurelien for the softfloat style questions:)

I think we should go ahead and define a float16 type. Also, at the moment the commonNaNToFloatX(floatYToCommonNaN()) idiom doesn't do anything to avoid signaling NaNs showing up in the output. On ARM this got fixed by having the helper.c wrappers call float*_maybe_silence_nan() but maybe we should do it in the generic softfloat code?

ie instead of:

    if ( mantissa )
        return make_float32(
            ( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) );
    else
        return float32_default_nan;

do:
float32 r = make_float32(

            ( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) );
   if (!mantissa) {
      /* target specific, !SNAN_BIT_IS_ONE targets probably
       * just set the QNAN bit (true for ARM, SPARC)
       * others likely return the default NaN ?
       */
   } else {
      return float32_maybe_silence_nan(r);

}

I'm pretty sure the existing code is wrong for SPARC, for instance, which is supposed to return a float32 qNaN with just the QNAN bit set if it is presented with a float64 signalling NaN all of whose non-zero mantissa bits are at the bottom end and don't make it into the float32. (ARM dodges a bullet here because as it happens our float32 default_nan has only the QNAN bit set, but SPARC's has all the mantissa bits set.)

Opinions?

-- PMM

Aurelien Jarno

2011-02-09 18:35 UTC #5 lore raw

Hi,

Quoted 6 lines · Peter Maydell

Quoted 4 lines · Peter Maydell
Quoted 1 line · Christophe Lyon
- this means implementing float16ToCommonNaN, thus float16_is_signaling_nan()
...like that, yes.

Quoted 7 lines

I have a slightly-tested implementation of these functions on my hard disk now; I'll try to finish testing them tomorrow (my testing is still falling over on some inputs to VCVTT/VCVTB).
(cc: Aurelien for the softfloat style questions:)
I think we should go ahead and define a float16 type.

I fully agree with that. I have seen you have already sent patches about that, will look at them later.

Quoted 14 lines

Also, at the moment the commonNaNToFloatX(floatYToCommonNaN()) idiom doesn't do anything to avoid signaling NaNs showing up in the output. On ARM this got fixed by having the helper.c wrappers call float*_maybe_silence_nan() but maybe we should do it in the generic softfloat code?
ie instead of:
    if ( mantissa )
        return make_float32(
            ( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) );
    else
        return float32_default_nan;

Quoted 3 lines

do:

   float32 r = make_float32(
            ( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) );

On an unrelated note, if we changes in this function, it might be a good idea to use mantissa instead of a.high>>41. It's the same, but probably easier to read.

Quoted 17 lines

   if (!mantissa) {
      /* target specific, !SNAN_BIT_IS_ONE targets probably
       * just set the QNAN bit (true for ARM, SPARC)
       * others likely return the default NaN ?
       */
   } else {
      return float32_maybe_silence_nan(r);
   }
I'm pretty sure the existing code is wrong for SPARC, for instance, which is supposed to return a float32 qNaN with just the QNAN bit set if it is presented with a float64 signalling NaN all of whose non-zero mantissa bits are at the bottom end and don't make it into the float32. (ARM dodges a bullet here because as it happens our float32 default_nan has only the QNAN bit set, but SPARC's has all the mantissa bits set.)

Quoted 2 lines

Opinions?

It makes sense. I will play a bit with a real MIPS machine to see how it behaves and come back with my results.

Signature · 2 lines

Aurelien Jarno                          GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net

Peter Maydell

2011-02-09 18:56 UTC #6 lore raw

Quoted 16 lines · Aurelien Jarno

Quoted 15 lines · Peter Maydell
On 8 February 2011 18:53, Peter Maydell <[email protected]> wrote:
Also, at the moment the commonNaNToFloatX(floatYToCommonNaN()) idiom doesn't do anything to avoid signaling NaNs showing up in the output. On ARM this got fixed by having the helper.c wrappers call float*_maybe_silence_nan() but maybe we should do it
in the generic softfloat code?
ie instead of:
if ( mantissa )
return make_float32(
( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) ); else
return float32_default_nan;

Quoted 7 lines

Quoted 3 lines
do:
float32 r = make_float32(
( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) );
On an unrelated note, if we changes in this function, it might be a good idea to use mantissa instead of a.high>>41. It's the same, but probably easier to read.

Mmm, I noticed that (although I don't think I fixed it in my other patchset.)

Quoted 17 lines

Quoted 17 lines
if (!mantissa) {
/* target specific, !SNAN_BIT_IS_ONE targets probably
* just set the QNAN bit (true for ARM, SPARC)
* others likely return the default NaN ?
*/
} else {
return float32_maybe_silence_nan(r);
}
I'm pretty sure the existing code is wrong for SPARC, for instance, which is supposed to return a float32 qNaN with just the QNAN bit set if it is presented with a float64 signalling NaN all of whose non-zero mantissa bits are at the bottom end and don't make it into the float32. (ARM dodges a bullet here because as it happens our float32 default_nan has only the QNAN bit set, but SPARC's has all the mantissa bits set.)

Quoted 5 lines

Quoted 2 lines
Opinions?
It makes sense. I will play a bit with a real MIPS machine to see how it behaves and come back with my results.

Thanks. (The patchset I sent skips this issue and just treats float16 the same way target-arm already treats float32 etc, ie does the silencing in the helper wrapper function, because it seemed more useful to fix that without bogging it down in whether we want to do NaN handling in the softfloat core.)

-- PMM

Aurelien Jarno

2011-02-21 16:44 UTC #7 lore raw

Sorry to come back so long after, just found time to do some tests.

Quoted 21 lines · Peter Maydell

Quoted 20 lines · Aurelien Jarno
Quoted 18 lines · Peter Maydell
On 8 February 2011 18:53, Peter Maydell <[email protected]> wrote:
Also, at the moment the commonNaNToFloatX(floatYToCommonNaN()) idiom doesn't do anything to avoid signaling NaNs showing up in the output. On ARM this got fixed by having the helper.c wrappers call float*_maybe_silence_nan() but maybe we should do it
in the generic softfloat code?
ie instead of:
if ( mantissa )
return make_float32(
( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) ); else
return float32_default_nan;
do:
float32 r = make_float32(
( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) );

Quoted 12 lines

Quoted 3 lines
On an unrelated note, if we changes in this function, it might be a good idea to use mantissa instead of a.high>>41. It's the same, but probably easier to read.
Mmm, I noticed that (although I don't think I fixed it in my other patchset.)
Quoted 5 lines
Quoted 5 lines
if (!mantissa) {
/* target specific, !SNAN_BIT_IS_ONE targets probably
* just set the QNAN bit (true for ARM, SPARC)
* others likely return the default NaN ?
*/

For MIPS the current code is correct, it returns the default NaN in that case. For at least PowerPC, SPARC and x86 setting the qNaN bit is the thing to do. SH4 always returns the default NaN, so this code path is never triggered.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
} else {
return float32_maybe_silence_nan(r);

This part seems to work for at least PowerPC, SPARC and x86, but doesn't match the MIPS implementation which doesn't silence NaNs during a conversion (sNaN output for sNaN input). SH4 always returns the default NaN, so this code path is never triggered.

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
}
I'm pretty sure the existing code is wrong for SPARC, for instance, which is supposed to return a float32 qNaN with just the QNAN bit set if it is presented with a float64 signalling NaN all of whose non-zero mantissa bits are at the bottom end and don't make it into the float32. (ARM dodges a bullet here because as it happens our float32 default_nan has only the QNAN bit set, but SPARC's has all the mantissa bits set.)

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Opinions?

The (!mantissa) change seems the way to got, OTOH it's probably better to keep the (mantissa) case in target specific code.

Signature · 2 lines

Aurelien Jarno	                        GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net

Aurelien Jarno

2011-02-21 17:52 UTC #8 lore raw

Quoted 42 lines · Aurelien Jarno

Sorry to come back so long after, just found time to do some tests.
Quoted 21 lines · Peter Maydell
Quoted 20 lines · Aurelien Jarno
Quoted 18 lines · Peter Maydell
On 8 February 2011 18:53, Peter Maydell <[email protected]> wrote:
Also, at the moment the commonNaNToFloatX(floatYToCommonNaN()) idiom doesn't do anything to avoid signaling NaNs showing up in the output. On ARM this got fixed by having the helper.c wrappers call float*_maybe_silence_nan() but maybe we should do it
in the generic softfloat code?
ie instead of:
if ( mantissa )
return make_float32(
( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) ); else
return float32_default_nan;
do:
float32 r = make_float32(
( ( (bits32) a.sign )<<31 ) | 0x7F800000 | ( a.high>>41 ) );
Quoted 12 lines
Quoted 3 lines
On an unrelated note, if we changes in this function, it might be a good idea to use mantissa instead of a.high>>41. It's the same, but probably easier to read.
Mmm, I noticed that (although I don't think I fixed it in my other patchset.)
Quoted 5 lines
Quoted 5 lines
if (!mantissa) {
/* target specific, !SNAN_BIT_IS_ONE targets probably
* just set the QNAN bit (true for ARM, SPARC)
* others likely return the default NaN ?
*/
For MIPS the current code is correct, it returns the default NaN in that case. For at least PowerPC, SPARC and x86 setting the qNaN bit is the thing to do. SH4 always returns the default NaN, so this code path is never triggered.

Quoted 8 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
} else {
return float32_maybe_silence_nan(r);
This part seems to work for at least PowerPC, SPARC and x86, but doesn't match the MIPS implementation which doesn't silence NaNs during a conversion (sNaN output for sNaN input). SH4 always returns the default NaN, so this code path is never triggered.

Actually that part seems to depends on the implementations. I have seen 3 cases:
- sNaN are not silenced
- sNaN are silenced
- default NaN is returned

The MIPS manual doesn't specify this case precisely, however it seems to rule out the first case, which seems to be a bug of the silicon:

"An SNaN is never produced as a result value."

So I am fine if we go with float32_maybe_silence_nan()

Signature · 2 lines

Aurelien Jarno	                        GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net