[Qemu-devel] softfloat: fix NaN propagation for MIPS and PowerPC + cleanup

[Qemu-devel] softfloat: fix NaN propagation for MIPS and PowerPC + cleanup

[Aurelien Jarno]

This patch series start by a cleanup to remove dead HPPA code, and then
implement correct NaN propagation rules for MIPS and PowerPC, following
commit 354f211b1a49a7387929e22d6e63849fcba48f8a.

[Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

We don't have any HPPA target, so let's remove HPPA specific code. It
can be re-added when someone adds an HPPA target.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-specialize.h |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index f8f36f3..f23bd6a 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -30,7 +30,7 @@ these four paragraphs for those parts of this code that are retained.
 
 =============================================================================*/
 
-#if defined(TARGET_MIPS) || defined(TARGET_HPPA)
+#if defined(TARGET_MIPS)
 #define SNAN_BIT_IS_ONE		1
 #else
 #define SNAN_BIT_IS_ONE		0
@@ -63,8 +63,6 @@ typedef struct {
 #define float32_default_nan make_float32(0x7FFFFFFF)
 #elif defined(TARGET_POWERPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA)
 #define float32_default_nan make_float32(0x7FC00000)
-#elif defined(TARGET_HPPA)
-#define float32_default_nan make_float32(0x7FA00000)
 #elif SNAN_BIT_IS_ONE
 #define float32_default_nan make_float32(0x7FBFFFFF)
 #else
@@ -275,8 +273,6 @@ static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM)
 #define float64_default_nan make_float64(LIT64( 0x7FFFFFFFFFFFFFFF ))
 #elif defined(TARGET_POWERPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA)
 #define float64_default_nan make_float64(LIT64( 0x7FF8000000000000 ))
-#elif defined(TARGET_HPPA)
-#define float64_default_nan make_float64(LIT64( 0x7FF4000000000000 ))
 #elif SNAN_BIT_IS_ONE
 #define float64_default_nan make_float64(LIT64( 0x7FF7FFFFFFFFFFFF ))
 #else
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/6] softfloat: fix float{32, 64}_maybe_silence_nan() for MIPS

[Aurelien Jarno]

On targets that define sNaN with the sNaN bit as one, simply clearing
this bit may correspond to an infinite value.

Convert it to a default NaN if SNAN_BIT_IS_ONE, as it corresponds to
the MIPS implementation, the only emulated CPU with SNAN_BIT_IS_ONE.
When other CPU of this type are added, this might be updated to include
more cases.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-specialize.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index f23bd6a..31481e7 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -107,13 +107,13 @@ int float32_is_signaling_nan( float32 a_ )
 float32 float32_maybe_silence_nan( float32 a_ )
 {
     if (float32_is_signaling_nan(a_)) {
-        bits32 a = float32_val(a_);
 #if SNAN_BIT_IS_ONE
-        a &= ~(1 << 22);
+        return float32_default_nan;
 #else
+        bits32 a = float32_val(a_);
         a |= (1 << 22);
-#endif
         return make_float32(a);
+#endif
     }
     return a_;
 }
@@ -321,13 +321,13 @@ int float64_is_signaling_nan( float64 a_ )
 float64 float64_maybe_silence_nan( float64 a_ )
 {
     if (float64_is_signaling_nan(a_)) {
-        bits64 a = float64_val(a_);
 #if SNAN_BIT_IS_ONE
-        a &= ~LIT64( 0x0008000000000000 );
+        return float64_default_nan;
 #else
+        bits64 a = float64_val(a_);
         a |= LIT64( 0x0008000000000000 );
-#endif
         return make_float64(a);
+#endif
     }
     return a_;
 }
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/6] softfloat: add float{x80, 128}_maybe_silence_nan()

[Aurelien Jarno]

Add float{x80,128}_maybe_silence_nan() functions, they will be need by
propagateFloat{x80,128}NaN().

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-specialize.h |   38 ++++++++++++++++++++++++++++++++++++++
 fpu/softfloat.h            |    2 ++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index 31481e7..49e3cc2 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -470,6 +470,25 @@ int floatx80_is_signaling_nan( floatx80 a )
 }
 
 /*----------------------------------------------------------------------------
+| Returns a quiet NaN if the extended double-precision floating point value
+| `a' is a signaling NaN; otherwise returns `a'.
+*----------------------------------------------------------------------------*/
+
+floatx80 floatx80_maybe_silence_nan( floatx80 a )
+{
+    if (floatx80_is_signaling_nan(a)) {
+#if SNAN_BIT_IS_ONE
+        a.low = floatx80_default_nan_low;
+        a.high = floatx80_default_nan_high;
+#else
+        a.low |= LIT64( 0xC000000000000000 );
+        return a;
+#endif
+    }
+    return a;
+}
+
+/*----------------------------------------------------------------------------
 | Returns the result of converting the extended double-precision floating-
 | point NaN `a' to the canonical NaN format.  If `a' is a signaling NaN, the
 | invalid exception is raised.
@@ -601,6 +620,25 @@ int float128_is_signaling_nan( float128 a )
 }
 
 /*----------------------------------------------------------------------------
+| Returns a quiet NaN if the quadruple-precision floating point value `a' is
+| a signaling NaN; otherwise returns `a'.
+*----------------------------------------------------------------------------*/
+
+float128 float128_maybe_silence_nan( float128 a )
+{
+    if (float128_is_signaling_nan(a)) {
+#if SNAN_BIT_IS_ONE
+        a.low = float128_default_nan_low;
+        a.high = float128_default_nan_high;
+#else
+        a.high |= LIT64( 0x0000800000000000 );
+        return a;
+#endif
+    }
+    return a;
+}
+
+/*----------------------------------------------------------------------------
 | Returns the result of converting the quadruple-precision floating-point NaN
 | `a' to the canonical NaN format.  If `a' is a signaling NaN, the invalid
 | exception is raised.
diff --git a/fpu/softfloat.h b/fpu/softfloat.h
index 1f37877..f2104c6 100644
--- a/fpu/softfloat.h
+++ b/fpu/softfloat.h
@@ -439,6 +439,7 @@ int floatx80_le_quiet( floatx80, floatx80 STATUS_PARAM );
 int floatx80_lt_quiet( floatx80, floatx80 STATUS_PARAM );
 int floatx80_is_quiet_nan( floatx80 );
 int floatx80_is_signaling_nan( floatx80 );
+floatx80 floatx80_maybe_silence_nan( floatx80 );
 floatx80 floatx80_scalbn( floatx80, int STATUS_PARAM );
 
 INLINE floatx80 floatx80_abs(floatx80 a)
@@ -505,6 +506,7 @@ int float128_compare( float128, float128 STATUS_PARAM );
 int float128_compare_quiet( float128, float128 STATUS_PARAM );
 int float128_is_quiet_nan( float128 );
 int float128_is_signaling_nan( float128 );
+float128 float128_maybe_silence_nan( float128 );
 float128 float128_scalbn( float128, int STATUS_PARAM );
 
 INLINE float128 float128_abs(float128 a)
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/6] softfloat: use float{32, 64, x80, 128}_maybe_silence_nan()

[Aurelien Jarno]

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(-)

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)) {
-- 
1.7.2.3

[Qemu-devel] [PATCH 5/6] target-mips: Implement correct NaN propagation rules

[Aurelien Jarno]

Implement the correct NaN propagation rules for MIPS targets by
providing an appropriate pickNaN function.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-specialize.h |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index 4deb165..150500b 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -187,6 +187,33 @@ static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
         return 1;
     }
 }
+#elif defined (TARGET_MIPS)
+static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
+                    flag aIsLargerSignificand)
+{
+    /* According to MIPS specifications, if one of the two operands is
+     * a sNaN, a new qNaN has to be generated. This is done in
+     * floatXX_maybe_silence_nan(). For qNaN inputs the specifications
+     * says: "When possible, this QNaN result is one of the operand QNaN
+     * values." In practice it seems that most implementations choose
+     * the first operand if both operands are qNaN. In short this gives
+     * the following rules:
+     *  1. A if it is signaling
+     *  2. B if it is signaling
+     *  3. A (quiet)
+     *  4. B (quiet)
+     * A signaling NaN is always quietened before returning it.
+     */
+    if (aIsSNaN) {
+        return 0;
+    } else if (bIsSNaN) {
+        return 1;
+    } else if (aIsQNaN) {
+        return 0;
+    } else {
+        return 1;
+    }
+}
 #else
 static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
                     flag aIsLargerSignificand)
-- 
1.7.2.3

[Qemu-devel] [PATCH 6/6] target-ppc: Implement correct NaN propagation rules

[Aurelien Jarno]

Implement the correct NaN propagation rules for ARM targets by
providing an appropriate pickNaN function.

Also fix the #ifdef tests for default NaN definition, the correct name
is TARGET_PPC instead of TARGET_POWERPC.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat-specialize.h |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index 150500b..0f6dbd0 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -61,7 +61,7 @@ typedef struct {
 *----------------------------------------------------------------------------*/
 #if defined(TARGET_SPARC)
 #define float32_default_nan make_float32(0x7FFFFFFF)
-#elif defined(TARGET_POWERPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA)
+#elif defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA)
 #define float32_default_nan make_float32(0x7FC00000)
 #elif SNAN_BIT_IS_ONE
 #define float32_default_nan make_float32(0x7FBFFFFF)
@@ -214,6 +214,21 @@ static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
         return 1;
     }
 }
+#elif defined(TARGET_PPC)
+static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
+                   flag aIsLargerSignificand)
+{
+    /* PowerPC propagation rules:
+     *  1. A if it sNaN or qNaN
+     *  2. B if it sNaN or qNaN
+     * A signaling NaN is always quietened before returning it.
+     */
+    if (aIsSNaN || aIsQNaN) {
+        return 0;
+    } else {
+        return 1;
+    }
+}
 #else
 static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
                     flag aIsLargerSignificand)
@@ -292,7 +307,7 @@ static float32 propagateFloat32NaN( float32 a, float32 b STATUS_PARAM)
 *----------------------------------------------------------------------------*/
 #if defined(TARGET_SPARC)
 #define float64_default_nan make_float64(LIT64( 0x7FFFFFFFFFFFFFFF ))
-#elif defined(TARGET_POWERPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA)
+#elif defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA)
 #define float64_default_nan make_float64(LIT64( 0x7FF8000000000000 ))
 #elif SNAN_BIT_IS_ONE
 #define float64_default_nan make_float64(LIT64( 0x7FF7FFFFFFFFFFFF ))
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 2/6] softfloat: fix float{32, 64}_maybe_silence_nan() for MIPS

[Peter Maydell]

On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On targets that define sNaN with the sNaN bit as one, simply clearing
> this bit may correspond to an infinite value.
>
> Convert it to a default NaN if SNAN_BIT_IS_ONE, as it corresponds to
> the MIPS implementation, the only emulated CPU with SNAN_BIT_IS_ONE.
> When other CPU of this type are added, this might be updated to include
> more cases.

This patch doesn't apply to master:

> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  fpu/softfloat-specialize.h |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index f23bd6a..31481e7 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -107,13 +107,13 @@ int float32_is_signaling_nan( float32 a_ )
>  float32 float32_maybe_silence_nan( float32 a_ )
>  {
>     if (float32_is_signaling_nan(a_)) {
> -        bits32 a = float32_val(a_);

...on master this line is
        uint32_t a = float32_val(a_);

(different type) so the patch doesn't apply.

Other than that, looks OK. I think I'd like a comment somewhere
along the lines of
/* Rules for silencing a signaling NaN are target-specific. Typically
 * targets with !SNAN_BIT_IS_ONE use the rule that the NaN
 * is silenced by setting the bit. Targets where SNAN_BIT_IS_ONE
 * must do something more complicated, because clearing the
 * bit might turn a NaN into an infinity. This code is correct for
 * MIPS but new targets might need something different.
 */

Or you could have the #ifdefs be on TARGET_whatever so
that it's clear (because it won't compile) that adding a new
TARGET_FOO means you have to check behaviour in this
area. But I don't feel very strongly about that.

-- PMM

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Peter Maydell]

On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
> We don't have any HPPA target, so let's remove HPPA specific code. It
> can be re-added when someone adds an HPPA target.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Do we want to get rid of the one remaining TARGET_HPPA which
is in linux-user/syscall_defs.h?

-- PMM

Re: [Qemu-devel] [PATCH 2/6] softfloat: fix float{32, 64}_maybe_silence_nan() for MIPS

[Aurelien Jarno]

Peter Maydell a écrit :
> On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> On targets that define sNaN with the sNaN bit as one, simply clearing
>> this bit may correspond to an infinite value.
>>
>> Convert it to a default NaN if SNAN_BIT_IS_ONE, as it corresponds to
>> the MIPS implementation, the only emulated CPU with SNAN_BIT_IS_ONE.
>> When other CPU of this type are added, this might be updated to include
>> more cases.
> 
> This patch doesn't apply to master:
> 
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>>  fpu/softfloat-specialize.h |   12 ++++++------
>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
>> index f23bd6a..31481e7 100644
>> --- a/fpu/softfloat-specialize.h
>> +++ b/fpu/softfloat-specialize.h
>> @@ -107,13 +107,13 @@ int float32_is_signaling_nan( float32 a_ )
>>  float32 float32_maybe_silence_nan( float32 a_ )
>>  {
>>     if (float32_is_signaling_nan(a_)) {
>> -        bits32 a = float32_val(a_);
> 
> ...on master this line is
>         uint32_t a = float32_val(a_);
> 
> (different type) so the patch doesn't apply.

Oops, yes, my patch series should have started by a patch fixing types,
but i made a mistake selecting the commits to send. Will fix that in a v2.

> Other than that, looks OK. I think I'd like a comment somewhere
> along the lines of
> /* Rules for silencing a signaling NaN are target-specific. Typically
>  * targets with !SNAN_BIT_IS_ONE use the rule that the NaN
>  * is silenced by setting the bit. Targets where SNAN_BIT_IS_ONE
>  * must do something more complicated, because clearing the
>  * bit might turn a NaN into an infinity. This code is correct for
>  * MIPS but new targets might need something different.
>  */
> 
> Or you could have the #ifdefs be on TARGET_whatever so
> that it's clear (because it won't compile) that adding a new
> TARGET_FOO means you have to check behaviour in this
> area. But I don't feel very strongly about that.
> 

Ok, thanks for the review, will fix that.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

Peter Maydell a écrit :
> On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> We don't have any HPPA target, so let's remove HPPA specific code. It
>> can be re-added when someone adds an HPPA target.
>>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Do we want to get rid of the one remaining TARGET_HPPA which
> is in linux-user/syscall_defs.h?
> 

Thanks for the review. I personnally don't have a lot of interest in
linux-user, so I will let the linux-user maintainer (Cc) to decide.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 3/6] softfloat: add float{x80, 128}_maybe_silence_nan()

[Peter Maydell]

On 3 January 2011 14:34, Aurelien Jarno <aurelien@aurel32.net> wrote:
> Add float{x80,128}_maybe_silence_nan() functions, they will be need by
> propagateFloat{x80,128}NaN().
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH 4/6] softfloat: use float{32, 64, x80, 128}_maybe_silence_nan()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 4/6] softfloat: use float{32, 64, x80, 128}_maybe_silence_nan()

[Aurelien Jarno]

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.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Andreas Färber]

Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:

> We don't have any HPPA target, so let's remove HPPA specific code. It
> can be re-added when someone adds an HPPA target.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

There actually is such a project on SourceForge [1, 2].

Does it really hurt to leave TARGET_HPPA around?

Andreas

[1] http://hppaqemu.sourceforge.net/
[2] http://wiki.qemu.org/Features/HPPA

> ---
> fpu/softfloat-specialize.h |    6 +-----
> 1 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index f8f36f3..f23bd6a 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -30,7 +30,7 @@ these four paragraphs for those parts of this code  
> that are retained.
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ====================================================================*/
>
> -#if defined(TARGET_MIPS) || defined(TARGET_HPPA)
> +#if defined(TARGET_MIPS)
> #define SNAN_BIT_IS_ONE		1
> #else
> #define SNAN_BIT_IS_ONE		0
> @@ -63,8 +63,6 @@ typedef struct {
> #define float32_default_nan make_float32(0x7FFFFFFF)
> #elif defined(TARGET_POWERPC) || defined(TARGET_ARM) ||  
> defined(TARGET_ALPHA)
> #define float32_default_nan make_float32(0x7FC00000)
> -#elif defined(TARGET_HPPA)
> -#define float32_default_nan make_float32(0x7FA00000)
> #elif SNAN_BIT_IS_ONE
> #define float32_default_nan make_float32(0x7FBFFFFF)
> #else
> @@ -275,8 +273,6 @@ static float32 propagateFloat32NaN( float32 a,  
> float32 b STATUS_PARAM)
> #define float64_default_nan make_float64(LIT64( 0x7FFFFFFFFFFFFFFF ))
> #elif defined(TARGET_POWERPC) || defined(TARGET_ARM) ||  
> defined(TARGET_ALPHA)
> #define float64_default_nan make_float64(LIT64( 0x7FF8000000000000 ))
> -#elif defined(TARGET_HPPA)
> -#define float64_default_nan make_float64(LIT64( 0x7FF4000000000000 ))
> #elif SNAN_BIT_IS_ONE
> #define float64_default_nan make_float64(LIT64( 0x7FF7FFFFFFFFFFFF ))
> #else
> -- 
> 1.7.2.3
>
>

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
> Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
>
>> We don't have any HPPA target, so let's remove HPPA specific code. It
>> can be re-added when someone adds an HPPA target.
>>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>
> There actually is such a project on SourceForge [1, 2].

The project hasn't seen any commit for 1.5 year. It looks like dead.

> Does it really hurt to leave TARGET_HPPA around?

It means writing code for this target, in the current case for
floatXX_maybe_silence_NaN(). I don't see the point of writing and
maintaining unused code if we don't get the insurance the target is
going to be merged later. Unless someone volunteer to maintain this
code.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Andreas Färber]

Am 04.01.2011 um 21:07 schrieb Aurelien Jarno:

> On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
>> Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
>>
>>> We don't have any HPPA target, so let's remove HPPA specific code.  
>>> It
>>> can be re-added when someone adds an HPPA target.
>>>
>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>
>> There actually is such a project on SourceForge [1, 2].
>
> The project hasn't seen any commit for 1.5 year. It looks like dead.

As we have begun to collect in the forks thread, there's many multi- 
month-old repos around with features not in upstream. Even "dead"  
doesn't mean useless. Considering that linux-user is incomplete even  
on amd64, I don't see why we shouldn't have target-hppa in master.  
Then it would at least allow for compile-testing and would benefit  
from general refactoring rather than bitrotting. All it takes is  
someone to step up for upstreaming patches, and I do not feel  
qualified to volunteer for that architecture.

>> Does it really hurt to leave TARGET_HPPA around?
>
> It means writing code for this target, in the current case for
> floatXX_maybe_silence_NaN(). I don't see the point of writing and
> maintaining unused code if we don't get the insurance the target is
> going to be merged later. Unless someone volunteer to maintain this
> code.

For new code,

#elif defined(TARGET_HPPA)
#error Target not supported yet.
...

shouldn't be too much work if you already handle architecture  
specifics and is different from ripping out existing code, as you  
seemed to suggest for linux-user.

Andreas

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

On Tue, Jan 04, 2011 at 11:53:01PM +0100, Andreas Färber wrote:
> Am 04.01.2011 um 21:07 schrieb Aurelien Jarno:
>
>> On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
>>> Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
>>>
>>>> We don't have any HPPA target, so let's remove HPPA specific code.  
>>>> It
>>>> can be re-added when someone adds an HPPA target.
>>>>
>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>>
>>> There actually is such a project on SourceForge [1, 2].
>>
>> The project hasn't seen any commit for 1.5 year. It looks like dead.
>
> As we have begun to collect in the forks thread, there's many multi- 
> month-old repos around with features not in upstream. Even "dead"  
> doesn't mean useless. Considering that linux-user is incomplete even on 
> amd64, I don't see why we shouldn't have target-hppa in master. Then it 
> would at least allow for compile-testing and would benefit from general 
> refactoring rather than bitrotting. All it takes is someone to step up 
> for upstreaming patches, and I do not feel qualified to volunteer for 
> that architecture.

You are comparing apples and oranges, forks and dead code. linux-user on
x86_64 is able to run binaries. HPPA code has still to be ported to TCG.
Yes it still uses dyngen.

>>> Does it really hurt to leave TARGET_HPPA around?
>>
>> It means writing code for this target, in the current case for
>> floatXX_maybe_silence_NaN(). I don't see the point of writing and
>> maintaining unused code if we don't get the insurance the target is
>> going to be merged later. Unless someone volunteer to maintain this
>> code.
>
> For new code,
>
> #elif defined(TARGET_HPPA)
> #error Target not supported yet.
> ...
>
> shouldn't be too much work if you already handle architecture specifics 

That makes work, code less readable, without any benefit. The code should
be added when we have a real fork to reintegrate back.

Should we already start adding code provision about TARGET_DPTR (the
marvelous CPU I have dreamed last night)? And about TARGET_IPU (the one
I have dreamed the night before)?

> and is different from ripping out existing code, as you seemed to suggest 
> for linux-user.

Strange interpretation for "I personnally don't have a lot of interest
in linux-user, so I will let the linux-user maintainer (Cc) to decide."

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Andreas Färber]

Am 05.01.2011 um 00:56 schrieb Aurelien Jarno:

> On Tue, Jan 04, 2011 at 11:53:01PM +0100, Andreas Färber wrote:
>> Am 04.01.2011 um 21:07 schrieb Aurelien Jarno:
>>
>>> On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
>>>> Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
>>>>
>>>>> We don't have any HPPA target, so let's remove HPPA specific code.
>>>>> It
>>>>> can be re-added when someone adds an HPPA target.
>>>>>
>>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>>>
>>>> There actually is such a project on SourceForge [1, 2].
>>>
>>> The project hasn't seen any commit for 1.5 year. It looks like dead.
>>
>> As we have begun to collect in the forks thread, there's many multi-
>> month-old repos around with features not in upstream. Even "dead"
>> doesn't mean useless. Considering that linux-user is incomplete  
>> even on
>> amd64, I don't see why we shouldn't have target-hppa in master.  
>> Then it
>> would at least allow for compile-testing and would benefit from  
>> general
>> refactoring rather than bitrotting. All it takes is someone to step  
>> up
>> for upstreaming patches, and I do not feel qualified to volunteer for
>> that architecture.
>
> You are comparing apples and oranges, forks and dead code. linux- 
> user on
> x86_64 is able to run binaries. HPPA code has still to be ported to  
> TCG.
> Yes it still uses dyngen.

Oh. I was pretty sure I saw TCG host support patches...

>>>> Does it really hurt to leave TARGET_HPPA around?
>>>
>>> It means writing code for this target, in the current case for
>>> floatXX_maybe_silence_NaN(). I don't see the point of writing and
>>> maintaining unused code if we don't get the insurance the target is
>>> going to be merged later. Unless someone volunteer to maintain this
>>> code.
>>
>> For new code,
>>
>> #elif defined(TARGET_HPPA)
>> #error Target not supported yet.
>> ...
>>
>> shouldn't be too much work if you already handle architecture  
>> specifics
>
> That makes work, code less readable, without any benefit. The code  
> should
> be added when we have a real fork to reintegrate back.
>
> Should we already start adding code provision about TARGET_DPTR (the
> marvelous CPU I have dreamed last night)? And about TARGET_IPU (the  
> one
> I have dreamed the night before)?
>
>> and is different from ripping out existing code, as you seemed to  
>> suggest
>> for linux-user.
>
> Strange interpretation for "I personnally don't have a lot of interest
> in linux-user, so I will let the linux-user maintainer (Cc) to  
> decide."

Really? I think you're completely missing my point.

Quote: "softfloat: remove HPPA specific code

We don't have any HPPA target, so let's remove HPPA specific code.  
[...]"

Quote: "> Do we want to get rid of the one remaining TARGET_HPPA which  
is in linux-user/syscall_defs.h?
[...] I will let the linux-user maintainer (Cc) [...] decide."

I'm saying that the justification of your patch and action towards  
Riku is wrong. Patch 1/6 should be dropped, don't remove code just  
because "we don't have any HPPA target". Because obviously if anyone  
rebases hppa.git to such a master HEAD, the code will be gone just as  
well.

What you've been interpreting into this is that you should go through  
hoops and newly add some TARGET_FOOBAR crap, which is not what I said.  
If you add some silence_magic_nan() and don't *know* how the code for  
HPPA should look like based on the pre-existent code, then I suggested  
to simply stub it out for such platforms (whether based on #elif  
defined(TARGET_foo) or a mere #else is pretty irrelevant to me). So in  
the end we will end up with patches that don't support hppa out of the  
box but which don't needlessly remove SNAN_BIT_IS_ONE and  
float{32,64}_default_nan(). I fail to see how these three macros hurt  
without digging deeper into your patches. If they do, then your patch  
1/6 is lacking a proper description!

Point is, don't remove valuable code just because it's not being used  
in upstream *yet*.

In that context, there would've been no need to bring the linux-user  
maintainer into this who could've complained earlier if HPPA caused  
him any trouble. I never stumbled across any prominent mention of hppa  
in his patch series.

Hope that explains,

Andreas

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

On Wed, Jan 05, 2011 at 09:15:43AM +0100, Andreas Färber wrote:
> Am 05.01.2011 um 00:56 schrieb Aurelien Jarno:
> 
> >On Tue, Jan 04, 2011 at 11:53:01PM +0100, Andreas Färber wrote:
> >>Am 04.01.2011 um 21:07 schrieb Aurelien Jarno:
> >>
> >>>On Tue, Jan 04, 2011 at 08:54:04PM +0100, Andreas Färber wrote:
> >>>>Am 03.01.2011 um 15:34 schrieb Aurelien Jarno:
> >>>>
> >>>>>We don't have any HPPA target, so let's remove HPPA specific code.
> >>>>>It
> >>>>>can be re-added when someone adds an HPPA target.
> >>>>>
> >>>>>Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> >>>>
> >>>>There actually is such a project on SourceForge [1, 2].
> >>>
> >>>The project hasn't seen any commit for 1.5 year. It looks like dead.
> >>
> >>As we have begun to collect in the forks thread, there's many multi-
> >>month-old repos around with features not in upstream. Even "dead"
> >>doesn't mean useless. Considering that linux-user is incomplete
> >>even on
> >>amd64, I don't see why we shouldn't have target-hppa in master.
> >>Then it
> >>would at least allow for compile-testing and would benefit from
> >>general
> >>refactoring rather than bitrotting. All it takes is someone to
> >>step up
> >>for upstreaming patches, and I do not feel qualified to volunteer for
> >>that architecture.
> >
> >You are comparing apples and oranges, forks and dead code. linux-
> >user on
> >x86_64 is able to run binaries. HPPA code has still to be ported
> >to TCG.
> >Yes it still uses dyngen.
> 
> Oh. I was pretty sure I saw TCG host support patches...

Yes, there is TCG *host* code in upstream, and this code has been updated
recently. 

But that's not the subject we are talking about HPPA guest support. 
target-hppa fork still (partially) uses dyngen code, which we don't 
support in upstream for more than *2 years*.

> >>>>Does it really hurt to leave TARGET_HPPA around?
> >>>
> >>>It means writing code for this target, in the current case for
> >>>floatXX_maybe_silence_NaN(). I don't see the point of writing and
> >>>maintaining unused code if we don't get the insurance the target is
> >>>going to be merged later. Unless someone volunteer to maintain this
> >>>code.
> >>
> >>For new code,
> >>
> >>#elif defined(TARGET_HPPA)
> >>#error Target not supported yet.
> >>...
> >>
> >>shouldn't be too much work if you already handle architecture
> >>specifics
> >
> >That makes work, code less readable, without any benefit. The code
> >should
> >be added when we have a real fork to reintegrate back.
> >
> >Should we already start adding code provision about TARGET_DPTR (the
> >marvelous CPU I have dreamed last night)? And about TARGET_IPU
> >(the one
> >I have dreamed the night before)?
> >
> >>and is different from ripping out existing code, as you seemed
> >>to suggest
> >>for linux-user.
> >
> >Strange interpretation for "I personnally don't have a lot of interest
> >in linux-user, so I will let the linux-user maintainer (Cc) to
> >decide."
> 
> Really? I think you're completely missing my point.
> 
> Quote: "softfloat: remove HPPA specific code
> 
> We don't have any HPPA target, so let's remove HPPA specific code.
> [...]"
> 
> Quote: "> Do we want to get rid of the one remaining TARGET_HPPA
> which is in linux-user/syscall_defs.h?
> [...] I will let the linux-user maintainer (Cc) [...] decide."
> 
> I'm saying that the justification of your patch and action towards
> Riku is wrong. Patch 1/6 should be dropped, don't remove code just
> because "we don't have any HPPA target". Because obviously if anyone
> rebases hppa.git to such a master HEAD, the code will be gone just
> as well.

We have forks still using dyngen. Rebasing them against HEAD will break
them (including this HPPA target). Still you didn't complain about 
dyngen removal. And this is only an example. QEMU evolves quite fast
recently, if you don't rebase regularly, your code becomes incompatible,
and useless quite soon.

> What you've been interpreting into this is that you should go
> through hoops and newly add some TARGET_FOOBAR crap, which is not
> what I said. If you add some silence_magic_nan() and don't *know*
> how the code for HPPA should look like based on the pre-existent
> code, then I suggested to simply stub it out for such platforms
> (whether based on #elif defined(TARGET_foo) or a mere #else is
> pretty irrelevant to me). So in the end we will end up with patches
> that don't support hppa out of the box but which don't needlessly
> remove SNAN_BIT_IS_ONE and float{32,64}_default_nan(). I fail to see
> how these three macros hurt without digging deeper into your
> patches. If they do, then your patch 1/6 is lacking a proper
> description!
> 
> Point is, don't remove valuable code just because it's not being
> used in upstream *yet*.

Please define "yet". I am fine dropping this patch if we know for sure
that some progress will be done on the HPPA target. But I am not fine
keeping useless code eternally.

I propose to drop this patch for now, I'll commit it in 6 months if
nothing has changed with regards to HPPA. That will make 2 years without
activity.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 6/6] target-ppc: Implement correct NaN propagation rules

[Nathan Froyd]

On Mon, Jan 03, 2011 at 03:34:33PM +0100, Aurelien Jarno wrote:
> Implement the correct NaN propagation rules for ARM targets by
> providing an appropriate pickNaN function.
> 
> Also fix the #ifdef tests for default NaN definition, the correct name
> is TARGET_PPC instead of TARGET_POWERPC.

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

> +     * A signaling NaN is always quietened before returning it.

I think "silenced" is more natural here, but I can understand preferring
"quiet" in keeping with NaN terminology.

-Nathan

[Qemu-devel] Re: [PATCH 6/6] target-ppc: Implement correct NaN propagation rules

[Alexander Graf]

On 03.01.2011, at 15:34, Aurelien Jarno wrote:

> Implement the correct NaN propagation rules for ARM targets by
> providing an appropriate pickNaN function.
> 
> Also fix the #ifdef tests for default NaN definition, the correct name
> is TARGET_PPC instead of TARGET_POWERPC.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Looks syntactically correct, no idea if it's semantically right too. I suppose you did verify that though.


Alex

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Stuart Brady]

On Wed, Jan 05, 2011 at 11:21:06AM +0100, Aurelien Jarno wrote:

> But that's not the subject we are talking about HPPA guest support. 
> target-hppa fork still (partially) uses dyngen code, which we don't 
> support in upstream for more than *2 years*.

I'd agree -- the fork is currently dead.  Whilst I do plan to revive it
at some point, I would support removal of those particular lines, as:

   Dead code is noise.  It doesn't contribute anything, so you may as
   well remove it.

   It's easy enough to add it back at a later date.

   Dead code is buggy code.  Previously, the code was quietening
   signaling NaNs incorrectly.  MIPS now uses a default quiet NaN,
   but HPPA would need a different fix.

   It's not even clear which of the two possible fixes should be applied
   (or whether both should be applied with a means of selecting the
   behaviour at run-time).  I gather that we need to clear the MSB of
   the significand, and then set its least-significant-but-one bit,
   either unconditionally, or perhaps only if the significand would
   otherwise be zero.  However, I don't yet know which of those two
   behaviours is implemented by hardware, and even if only one is
   implemented, I still feel we'd still need a comment explaining that
   the architecture's specification permits the alternate behaviour.

A few random thank-yous:

   I do really appreciate the effort to avoid removing lines that would
   only be added back at a later date -- if we had an HPPA target fork
   that was ready to be merged back in a week or two, then I'd argue that
   there's no point, but that's not the case.

   Thanks for the comments on sNaN quietening for HPPA.  I would hopefully
   read the PA1.1 spec thoroughly enough and test on real hardware upon
   reaching the point where floating point support is somewhere higher
   up on the TODO list... :-)  However, the comments can hardly hurt!

   What is important here is that the code be rewritten in a clean manner,
   so that there are no unnecessary obstacles to modifying SoftFloat to
   support new targets.  The patches that I've seen definitely seem to
   move in that direction, so I'm quite happy.  Of course, fixing those
   architectures that are already upstream is the main objective! -- and
   if this helps other forks, that can't be a bad thing.

I do have a few concerns regarding SoftFloat, though:

   FIXMEs should be left in the code (or a document maintained on the
   Wiki) to keep track of which architectures have been considered
   (which I believe are x86, arm, mips, ppc) and which ones haven't.
   This is in reference to one particular FIXME that was removed,
   but perhaps shouldn't have been.

   Is there any plan to deal with use of float*_is_quiet_nan(), where
   float*_is_any_nan() was intended?  These should really either be
   fixed (and tested), or if not, a FIXME should be added.

   Perhaps it would be worth documenting the IEEE 754-2008 behaviour,
   especially in any cases where we already happen to implement that
   behaviour?  (Once I've actually looked at IEEE 754-2008, I might
   be able to contribute something in this regard.)

   I do appreciate that there's still work to be done -- my intent here is
   just to make sure that nothing's in danger of slipping through the net.

Regarding the TARGET_HPPA definition in linux-user:

   In my opinion, the reference to TARGET_HPPA in the definition of
   target_flock64 in linux-user/syscall_defs.h should also be ditched,
   although whether it's worth anyone's time submitting a patch to remove
   that, I'm not really sure.

   I would also argue that the handling of padding in linux-user is less
   than ideal.  The problem here seems to be that as fields have different
   sizes (or may be absent entirely) on different architectures, different
   padding is required.  Rather than a laundry list of targets, this would
   best handled automatically, even if this would required rather evil
   macros or code generation... perhaps I should give this a go?

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Peter Maydell]

On 5 January 2011 23:13, Stuart Brady <sdb@zubnet.me.uk> wrote:
> I do have a few concerns regarding SoftFloat, though:
>
>   FIXMEs should be left in the code (or a document maintained on the
>   Wiki) to keep track of which architectures have been considered
>   (which I believe are x86, arm, mips, ppc) and which ones haven't.
>   This is in reference to one particular FIXME that was removed,
>   but perhaps shouldn't have been.

Which one? The only one I know I removed was the one in
the patch that implemented the thing it was complaining about,
but perhaps I took out another by accident...

>   Is there any plan to deal with use of float*_is_quiet_nan(), where
>   float*_is_any_nan() was intended?  These should really either be
>   fixed (and tested), or if not, a FIXME should be added.

I was planning to do the ARM related ones, at least (although
they are in the pretty-much-dead FPE-emulation code for
linux user-mode).

I would be more inclined to track that sort of thing in a bug tracker
than by sprinkling the code with FIXME comments.

-- PMM

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Andreas Färber]

Am 05.01.2011 um 11:21 schrieb Aurelien Jarno:

> On Wed, Jan 05, 2011 at 09:15:43AM +0100, Andreas Färber wrote:
>> Am 05.01.2011 um 00:56 schrieb Aurelien Jarno:
>>
>>> HPPA code has still to be ported
>>> to TCG.
>>> Yes it still uses dyngen.
>>
>> Oh. I was pretty sure I saw TCG host support patches...
>
> Yes, there is TCG *host* code in upstream, and this code has been  
> updated
> recently.
>
> But that's not the subject we are talking about HPPA guest support.

[Thanks, I do know the difference. Obviously, by inference I assumed  
that the target would then use TCG as well. Repo doesn't make it  
obvious what the changes to qemu.git are, since it was merged, not  
rebased.]

> We have forks still using dyngen. Rebasing them against HEAD will  
> break
> them (including this HPPA target). Still you didn't complain about
> dyngen removal.

For the record, I did. Replacing dyngen with TCG was a good step, but  
it broke Darwin/ppc host for MONTHS.

I also did for kqemu. Its removal effectively means NO acceleration on  
(Open)Solaris and Haiku.

And I'm curious how Windows support will work out. We have seen some  
patches lately (one yet to be reviewed comes to mind) but no official  
maintainer yet, which was demanded by Anthony to not drop Win32. On  
the other hand, AdaCore and CodeSourcery have spoken up as commercial  
Win32 users.

Anyway, for target-ppc, me and others helped in a concerted effort to  
get things converted to TCG and reviewed. Might work for other  
projects, too.

> And this is only an example. QEMU evolves quite fast
> recently, if you don't rebase regularly, your code becomes  
> incompatible,
> and useless quite soon.

Which is why I've volunteered to step up as PReP maintainer. Can't do  
that for all projects, but we should encourage people to push their  
favorite features into upstream before things diverge more making it  
even more complicated.

This was sort-of evoked by the "1st QEMU Users Forum" thread, which  
expressed a sentiment that the qemu-devel community had shifted  
towards KVM/virtualization and were neglecting the emulation part of  
QEMU.

> I'll commit it in 6 months if
> nothing has changed with regards to HPPA.

Just pointing out that there were 5 months without activity from  
yourself last year. It happens, Real Life can be gripping.

> That will make 2 years without
> activity.

My haiku-user project is still in its infancy for 1 year now, with the  
ELF runtime_loader not yet fully ported, since I dedicated more time  
to getting ppc system emulation working. While I try to emulate ppc,  
the Haiku/ppc kernel gets neglected, etc. Two years go by really  
quickly when you're busy!

Cheers,

Andreas

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

On Wed, Jan 05, 2011 at 11:13:06PM +0000, Stuart Brady wrote:
> On Wed, Jan 05, 2011 at 11:21:06AM +0100, Aurelien Jarno wrote:
> 
> > But that's not the subject we are talking about HPPA guest support. 
> > target-hppa fork still (partially) uses dyngen code, which we don't 
> > support in upstream for more than *2 years*.
> 
> I'd agree -- the fork is currently dead.  Whilst I do plan to revive it
> at some point, I would support removal of those particular lines, as:
> 
>    Dead code is noise.  It doesn't contribute anything, so you may as
>    well remove it.
> 
>    It's easy enough to add it back at a later date.
> 
>    Dead code is buggy code.  Previously, the code was quietening
>    signaling NaNs incorrectly.  MIPS now uses a default quiet NaN,
>    but HPPA would need a different fix.
> 
>    It's not even clear which of the two possible fixes should be applied
>    (or whether both should be applied with a means of selecting the
>    behaviour at run-time).  I gather that we need to clear the MSB of
>    the significand, and then set its least-significant-but-one bit,
>    either unconditionally, or perhaps only if the significand would
>    otherwise be zero.  However, I don't yet know which of those two
>    behaviours is implemented by hardware, and even if only one is
>    implemented, I still feel we'd still need a comment explaining that
>    the architecture's specification permits the alternate behaviour.
> 
> A few random thank-yous:
> 
>    I do really appreciate the effort to avoid removing lines that would
>    only be added back at a later date -- if we had an HPPA target fork
>    that was ready to be merged back in a week or two, then I'd argue that
>    there's no point, but that's not the case.
> 
>    Thanks for the comments on sNaN quietening for HPPA.  I would hopefully
>    read the PA1.1 spec thoroughly enough and test on real hardware upon
>    reaching the point where floating point support is somewhere higher
>    up on the TODO list... :-)  However, the comments can hardly hurt!
> 
>    What is important here is that the code be rewritten in a clean manner,
>    so that there are no unnecessary obstacles to modifying SoftFloat to
>    support new targets.  The patches that I've seen definitely seem to
>    move in that direction, so I'm quite happy.  Of course, fixing those
>    architectures that are already upstream is the main objective! -- and
>    if this helps other forks, that can't be a bad thing.
> 
> I do have a few concerns regarding SoftFloat, though:
> 
>    FIXMEs should be left in the code (or a document maintained on the
>    Wiki) to keep track of which architectures have been considered
>    (which I believe are x86, arm, mips, ppc) and which ones haven't.
>    This is in reference to one particular FIXME that was removed,
>    but perhaps shouldn't have been.
> 
>    Is there any plan to deal with use of float*_is_quiet_nan(), where
>    float*_is_any_nan() was intended?  These should really either be
>    fixed (and tested), or if not, a FIXME should be added.

The problem with these functions is that they are used in target-*/
and not directly part of the softfloat code.

I have reviewed MIPS and PowerPC targets, they both use these functions
correctly.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

On Thu, Jan 06, 2011 at 02:10:36PM +0100, Andreas Färber wrote:
> Am 05.01.2011 um 11:21 schrieb Aurelien Jarno:
>> I'll commit it in 6 months if
>> nothing has changed with regards to HPPA.
>
> Just pointing out that there were 5 months without activity from  
> yourself last year. It happens, Real Life can be gripping.

Fully agreed that it can happen. During this 5 months, I have seen
a number of patches I was not very happy with. However I considered 
that not doing any QEMU work, I could not really complain about those
changes.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Peter Maydell]

On 6 January 2011 14:35, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Wed, Jan 05, 2011 at 11:13:06PM +0000, Stuart Brady wrote:
>>    Is there any plan to deal with use of float*_is_quiet_nan(), where
>>    float*_is_any_nan() was intended?  These should really either be
>>    fixed (and tested), or if not, a FIXME should be added.
>
> The problem with these functions is that they are used in target-*/
> and not directly part of the softfloat code.
>
> I have reviewed MIPS and PowerPC targets, they both use these functions
> correctly.

MIPS looks OK. However PPC has this in helper_compute_fprf():

    if (unlikely(float64_is_quiet_nan(farg.d))) {
        if (float64_is_signaling_nan(farg.d)) {
            /* Signaling NaN: flags are undefined */
            ret = 0x00;
        } else {
            /* Quiet NaN */
            ret = 0x11;
        }

which is definitely wrong because the first case can't be reached.
The outer if should be is_any_nan(), I think.

In helper_fnmadd() and helper_fnmsub():
        if (likely(!float64_is_quiet_nan(farg1.d)))
            farg1.d = float64_chs(farg1.d);

is I think OK but somebody else might like to check.

Similarly I'm dubious about uses in helper_fsel, helper_fcmpu
and helper_fcmpo, efsctsi, efsctui, efsctsiz, efsctuiz, efsctsf,
efsctuf and all the helper_efd* functions. I haven't actually
checked them all, but for instance efdctsi in the Power ISA
2.03 spec says "NaNs are converted as though they were zero",
but qemu's code says:
    /* NaN are not treated the same way IEEE 754 does */
    if (unlikely(float64_is_quiet_nan(u.d)))
        return 0;

which is not going to do the right thing for signaling NaNs.

-- PMM

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Stuart Brady]

On Thu, Jan 06, 2011 at 08:58:17AM +0000, Peter Maydell wrote:
> On 5 January 2011 23:13, Stuart Brady <sdb@zubnet.me.uk> wrote:
> > I do have a few concerns regarding SoftFloat, though:
> >
> >   FIXMEs should be left in the code (or a document maintained on the
> >   Wiki) to keep track of which architectures have been considered
> >   (which I believe are x86, arm, mips, ppc) and which ones haven't.
> >   This is in reference to one particular FIXME that was removed,
> >   but perhaps shouldn't have been.
> 
> Which one? The only one I know I removed was the one in
> the patch that implemented the thing it was complaining about,
> but perhaps I took out another by accident...

The patch is question was:

   softfloat: Implement flushing input denormals to zero

The comment is:

   /* FIXME: Flush-To-Zero only effects results.  Denormal inputs should
      also be flushed to zero.  */

Do you know whether ARM is the only target architecture supported by
QEMU that requires this behaviour?

If there are any architectures where we simply don't know whether the
current behaviour is correct, we should document that somewhere.

For any target-specific behaviour, I really feel that we should have
architectures listed explicitly rather than having using a default
#else case (and that the #else case should simply contain a #error),
as it may be worth guarding against any newly added targets incorrectly
picking the default behaviour (as has happened in the past).

Presumably, sNaNs on any target where they are indicated by a zero bit
should be quietened by simply setting that bit.  However, if a target
doesn't define SNAN_BIT_IS_ONE, that may simply be because it was
forgotten.  This is why I don't like using #ifndef in general...
much better, IMO, to define as '0' or '1', and then -Werror=undef can
catch anything that gets overlooked.  (Except now the problem is any
erroneous use of #ifdef with such definitions that might creep in...)

> >   Is there any plan to deal with use of float*_is_quiet_nan(), where
> >   float*_is_any_nan() was intended?  These should really either be
> >   fixed (and tested), or if not, a FIXME should be added.
> 
> I was planning to do the ARM related ones, at least (although
> they are in the pretty-much-dead FPE-emulation code for
> linux user-mode).
> 
> I would be more inclined to track that sort of thing in a bug tracker
> than by sprinkling the code with FIXME comments.

I'd definitely agree that FIXMEs are not as good as bug reports, and/or
possibly even a Features/SoftFloat page on the Wiki if appropriate.

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Peter Maydell]

On 6 January 2011 18:13, Stuart Brady <sdb@zubnet.me.uk> wrote:
> On Thu, Jan 06, 2011 at 08:58:17AM +0000, Peter Maydell wrote:
>> On 5 January 2011 23:13, Stuart Brady <sdb@zubnet.me.uk> wrote:
>> > I do have a few concerns regarding SoftFloat, though:
>> >
>> >   FIXMEs should be left in the code (or a document maintained on the
>> >   Wiki) to keep track of which architectures have been considered
>> >   (which I believe are x86, arm, mips, ppc) and which ones haven't.
>> >   This is in reference to one particular FIXME that was removed,
>> >   but perhaps shouldn't have been.
>>
>> Which one? The only one I know I removed was the one in
>> the patch that implemented the thing it was complaining about,
>> but perhaps I took out another by accident...
>
> The patch is question was:
>
>   softfloat: Implement flushing input denormals to zero
>
> The comment is:
>
>   /* FIXME: Flush-To-Zero only effects results.  Denormal inputs should
>      also be flushed to zero.  */

The point of that FIXME is that it is saying "softfloat doesn't implement
the feature of flushing denormal inputs to zero". The patch implements
that feature in softfloat. Therefore the FIXME should be removed,
because it has been fixed :-)

> Do you know whether ARM is the only target architecture supported by
> QEMU that requires this behaviour?

As I said in the patch series cover letter, I suspect you could implement
a MIPS FCCR bit using the softfloat feature the patch adds. But that's
totally irrelevant to whether that FIXME comment should be deleted,
because that FIXME isn't about any architecture but about a softfloat
feature (or lack of it). Existing architectures are no more or less broken
than they were before, because they are unaffected by the patch series.

> If there are any architectures where we simply don't know whether the
> current behaviour is correct, we should document that somewhere.

That would be all of them, I rather suspect. I don't imagine anybody
has run any of the qemu emulated models through a rigorous
validation process for any architecture, so we can't say "we know
this is correct". I'm going through fixing things for ARM as I encounter
them and I have a huge list of corner cases where we don't do the
right thing.

>> >   Is there any plan to deal with use of float*_is_quiet_nan(), where
>> >   float*_is_any_nan() was intended?  These should really either be
>> >   fixed (and tested), or if not, a FIXME should be added.
>>
>> I was planning to do the ARM related ones, at least (although
>> they are in the pretty-much-dead FPE-emulation code for
>> linux user-mode).

I've now submitted a patchset to do that.

-- PMM

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
> On 6 January 2011 14:35, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Wed, Jan 05, 2011 at 11:13:06PM +0000, Stuart Brady wrote:
> >>    Is there any plan to deal with use of float*_is_quiet_nan(), where
> >>    float*_is_any_nan() was intended?  These should really either be
> >>    fixed (and tested), or if not, a FIXME should be added.
> >
> > The problem with these functions is that they are used in target-*/
> > and not directly part of the softfloat code.
> >
> > I have reviewed MIPS and PowerPC targets, they both use these functions
> > correctly.
> 
> MIPS looks OK. However PPC has this in helper_compute_fprf():
> 
>     if (unlikely(float64_is_quiet_nan(farg.d))) {
>         if (float64_is_signaling_nan(farg.d)) {
>             /* Signaling NaN: flags are undefined */
>             ret = 0x00;
>         } else {
>             /* Quiet NaN */
>             ret = 0x11;
>         }
> 
> which is definitely wrong because the first case can't be reached.
> The outer if should be is_any_nan(), I think.

Correct.

> In helper_fnmadd() and helper_fnmsub():
>         if (likely(!float64_is_quiet_nan(farg1.d)))
>             farg1.d = float64_chs(farg1.d);
> 
> is I think OK but somebody else might like to check.

After reading the manual again, it seems float64_is_nan() should be used
here. The idea is that fnmadd returns the negated value of fmadd, and
that NaN should be propagated as fnmadd was a single instruction. In
QEMU chs changes the sign bit even if the value is NaN. Quoting the
manual:

| This instruction produces the same result as would be obtained by 
| using the Floating Multiply-Add instruction and then negating the 
| result, with the following exceptions. 
| * QNaNs propagate with no effect on their "sign" bit.
| * QNaNs that are generated as the result of a disabled Invalid 
|   Operation Exception have a "sign" bit of 0.
| * SNaNs that are converted to QNaNs as the result of a disabled 
| Invalid Operation Exception retain the "sign”"bit of the SNaN.

> Similarly I'm dubious about uses in helper_fsel, helper_fcmpu
> and helper_fcmpo, 

I confirm the issue for this three helpers, tested on real hardware.

> efsctsi, efsctui, efsctsiz, efsctuiz, efsctsf,
> efsctuf and all the helper_efd* functions. I haven't actually
> checked them all, but for instance efdctsi in the Power ISA
> 2.03 spec says "NaNs are converted as though they were zero",
> but qemu's code says:
>     /* NaN are not treated the same way IEEE 754 does */
>     if (unlikely(float64_is_quiet_nan(u.d)))
>         return 0;
> 
> which is not going to do the right thing for signaling NaNs.
> 

Also correct.

Looks like I have read the code a bit quickly... Thanks for looking at
it, I will send a patch later.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Stuart Brady]

On Thu, Jan 06, 2011 at 06:43:28PM +0000, Peter Maydell wrote:
> On 6 January 2011 18:13, Stuart Brady <sdb@zubnet.me.uk> wrote:
> > On Thu, Jan 06, 2011 at 08:58:17AM +0000, Peter Maydell wrote:
> >> On 5 January 2011 23:13, Stuart Brady <sdb@zubnet.me.uk> wrote:
> >> > I do have a few concerns regarding SoftFloat, though:
> >> >
> >> >   FIXMEs should be left in the code (or a document maintained on the
> >> >   Wiki) to keep track of which architectures have been considered
> >> >   (which I believe are x86, arm, mips, ppc) and which ones haven't.
> >> >   This is in reference to one particular FIXME that was removed,
> >> >   but perhaps shouldn't have been.
[...]
> >   /* FIXME: Flush-To-Zero only effects results.  Denormal inputs should
> >      also be flushed to zero.  */
> 
> The point of that FIXME is that it is saying "softfloat doesn't implement
> the feature of flushing denormal inputs to zero". The patch implements
> that feature in softfloat. Therefore the FIXME should be removed,
> because it has been fixed :-)

Agreed, although note that I never insisted that the FIXME be kept in
this instance -- I was just concerned that this might be forgotten
for other targets.

If we accept that those targets are likely to be buggy, and that this
is an issue for the maintainers of those targets, that sounds fine,
but I just thought I'd ask.

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Nathan Froyd]

On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
> Similarly I'm dubious about uses in helper_fsel, helper_fcmpu
> and helper_fcmpo, efsctsi, efsctui, efsctsiz, efsctuiz, efsctsf,
> efsctuf and all the helper_efd* functions. I haven't actually
> checked them all, but for instance efdctsi in the Power ISA
> 2.03 spec says "NaNs are converted as though they were zero",
> but qemu's code says:
>     /* NaN are not treated the same way IEEE 754 does */
>     if (unlikely(float64_is_quiet_nan(u.d)))
>         return 0;
> 
> which is not going to do the right thing for signaling NaNs.

I think you are correct about fsel, fcmpu, and fcmpo.

The E500 FP instructions are broken for various corner cases (and there
are a lot of them, because E500 is screwy).  I've been meaning to go
through and fix them up, but haven't taken the time to do so.

-Nathan

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Peter Maydell]

On 6 January 2011 18:48, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
>> In helper_fnmadd() and helper_fnmsub():
>>         if (likely(!float64_is_quiet_nan(farg1.d)))
>>             farg1.d = float64_chs(farg1.d);
>>
>> is I think OK but somebody else might like to check.
>
> After reading the manual again, it seems float64_is_nan() should be used
> here. The idea is that fnmadd returns the negated value of fmadd, and
> that NaN should be propagated as fnmadd was a single instruction. In
> QEMU chs changes the sign bit even if the value is NaN. Quoting the
> manual:
>
> | This instruction produces the same result as would be obtained by
> | using the Floating Multiply-Add instruction and then negating the
> | result, with the following exceptions.
> | * QNaNs propagate with no effect on their "sign" bit.
> | * QNaNs that are generated as the result of a disabled Invalid
> |   Operation Exception have a "sign" bit of 0.
> | * SNaNs that are converted to QNaNs as the result of a disabled
> | Invalid Operation Exception retain the "sign”"bit of the SNaN.

Yes, I read that text, but I kind of convinced myself that at
that point in the code it wasn't possible to get a signalling NaN
(since they've been handled earlier). Still, the _any_nan() check
is probably clearer code.

-- PMM

Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

On Thu, Jan 06, 2011 at 09:19:13PM +0000, Peter Maydell wrote:
> On 6 January 2011 18:48, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
> >> In helper_fnmadd() and helper_fnmsub():
> >>         if (likely(!float64_is_quiet_nan(farg1.d)))
> >>             farg1.d = float64_chs(farg1.d);
> >>
> >> is I think OK but somebody else might like to check.
> >
> > After reading the manual again, it seems float64_is_nan() should be used
> > here. The idea is that fnmadd returns the negated value of fmadd, and
> > that NaN should be propagated as fnmadd was a single instruction. In
> > QEMU chs changes the sign bit even if the value is NaN. Quoting the
> > manual:
> >
> > | This instruction produces the same result as would be obtained by
> > | using the Floating Multiply-Add instruction and then negating the
> > | result, with the following exceptions.
> > | * QNaNs propagate with no effect on their "sign" bit.
> > | * QNaNs that are generated as the result of a disabled Invalid
> > |   Operation Exception have a "sign" bit of 0.
> > | * SNaNs that are converted to QNaNs as the result of a disabled
> > | Invalid Operation Exception retain the "sign”"bit of the SNaN.
> 
> Yes, I read that text, but I kind of convinced myself that at
> that point in the code it wasn't possible to get a signalling NaN
> (since they've been handled earlier). Still, the _any_nan() check
> is probably clearer code.
> 

For the long term I plan to add softfloat to have more granularity in
the invalid exception flags so that we can implement most PowerPC FP
without checking the operand first, and just looking at the softfloat 
exception flags. This will use this code path, so that's better to fix
it as it is spotted.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net