[Qemu-devel] [PATCH 0/3] target-ppc: improve FPU emulation

[Qemu-devel] [PATCH 0/3] target-ppc: improve FPU emulation

[Aurelien Jarno]

The PowerPC CPU has a lot more precise FPU exception than the IEEE754
specifications. 7 independant exceptions corresponding to the invalid
exception IEEE754. They are currently not implemented in softfloat code,
though they are detected to compute the result, that's why the target-ppc
code checks for the operands before calling the softfloat code.

This patch series fixes some issues with the FPU emulation, though the
long term solution is to implement the various flags in the softfloat 
code, and rely to compute the result.

Aurelien Jarno (3):
  target-ppc: remove PRECISE_EMULATION define
  target-ppc: fix default qNaN
  target-ppc: fix sNaN propagation

 target-ppc/exec.h      |    3 -
 target-ppc/op_helper.c |  203 +++++++++++++++++++++---------------------------
 2 files changed, 88 insertions(+), 118 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/3] target-ppc: remove PRECISE_EMULATION define

[Aurelien Jarno]

The PRECISE_EMULATION is "hardcoded" to one in target-ppc/exec.h and not
something easily tunable. Remove it and non-precise emulation code as
it doesn't make a noticeable difference in speed. People wanting speed
improvement should use softfloat-native instead.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/exec.h      |    3 --
 target-ppc/op_helper.c |   58 +++++++++--------------------------------------
 2 files changed, 11 insertions(+), 50 deletions(-)

diff --git a/target-ppc/exec.h b/target-ppc/exec.h
index 44cc5e9..4688ef5 100644
--- a/target-ppc/exec.h
+++ b/target-ppc/exec.h
@@ -26,9 +26,6 @@
 #include "cpu.h"
 #include "exec-all.h"
 
-/* Precise emulation is needed to correctly emulate exception flags */
-#define USE_PRECISE_EMULATION 1
-
 register struct CPUPPCState *env asm(AREG0);
 
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 89be0f4..858877e 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -974,7 +974,7 @@ uint64_t helper_fadd (uint64_t arg1, uint64_t arg2)
 
     farg1.ll = arg1;
     farg2.ll = arg2;
-#if USE_PRECISE_EMULATION
+
     if (unlikely(float64_is_signaling_nan(farg1.d) ||
                  float64_is_signaling_nan(farg2.d))) {
         /* sNaN addition */
@@ -986,9 +986,7 @@ uint64_t helper_fadd (uint64_t arg1, uint64_t arg2)
     } else {
         farg1.d = float64_add(farg1.d, farg2.d, &env->fp_status);
     }
-#else
-    farg1.d = float64_add(farg1.d, farg2.d, &env->fp_status);
-#endif
+
     return farg1.ll;
 }
 
@@ -999,8 +997,7 @@ uint64_t helper_fsub (uint64_t arg1, uint64_t arg2)
 
     farg1.ll = arg1;
     farg2.ll = arg2;
-#if USE_PRECISE_EMULATION
-{
+
     if (unlikely(float64_is_signaling_nan(farg1.d) ||
                  float64_is_signaling_nan(farg2.d))) {
         /* sNaN subtraction */
@@ -1012,10 +1009,7 @@ uint64_t helper_fsub (uint64_t arg1, uint64_t arg2)
     } else {
         farg1.d = float64_sub(farg1.d, farg2.d, &env->fp_status);
     }
-}
-#else
-    farg1.d = float64_sub(farg1.d, farg2.d, &env->fp_status);
-#endif
+
     return farg1.ll;
 }
 
@@ -1026,7 +1020,7 @@ uint64_t helper_fmul (uint64_t arg1, uint64_t arg2)
 
     farg1.ll = arg1;
     farg2.ll = arg2;
-#if USE_PRECISE_EMULATION
+
     if (unlikely(float64_is_signaling_nan(farg1.d) ||
                  float64_is_signaling_nan(farg2.d))) {
         /* sNaN multiplication */
@@ -1038,9 +1032,7 @@ uint64_t helper_fmul (uint64_t arg1, uint64_t arg2)
     } else {
         farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
     }
-#else
-    farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
-#endif
+
     return farg1.ll;
 }
 
@@ -1051,7 +1043,7 @@ uint64_t helper_fdiv (uint64_t arg1, uint64_t arg2)
 
     farg1.ll = arg1;
     farg2.ll = arg2;
-#if USE_PRECISE_EMULATION
+
     if (unlikely(float64_is_signaling_nan(farg1.d) ||
                  float64_is_signaling_nan(farg2.d))) {
         /* sNaN division */
@@ -1065,9 +1057,7 @@ uint64_t helper_fdiv (uint64_t arg1, uint64_t arg2)
     } else {
         farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);
     }
-#else
-    farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);
-#endif
+
     return farg1.ll;
 }
 
@@ -1116,12 +1106,10 @@ uint64_t helper_fctiw (uint64_t arg)
         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXCVI);
     } else {
         farg.ll = float64_to_int32(farg.d, &env->fp_status);
-#if USE_PRECISE_EMULATION
         /* XXX: higher bits are not supposed to be significant.
          *     to make tests easier, return the same as a real PowerPC 750
          */
         farg.ll |= 0xFFF80000ULL << 32;
-#endif
     }
     return farg.ll;
 }
@@ -1140,12 +1128,10 @@ uint64_t helper_fctiwz (uint64_t arg)
         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXCVI);
     } else {
         farg.ll = float64_to_int32_round_to_zero(farg.d, &env->fp_status);
-#if USE_PRECISE_EMULATION
         /* XXX: higher bits are not supposed to be significant.
          *     to make tests easier, return the same as a real PowerPC 750
          */
         farg.ll |= 0xFFF80000ULL << 32;
-#endif
     }
     return farg.ll;
 }
@@ -1245,7 +1231,7 @@ uint64_t helper_fmadd (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg1.ll = arg1;
     farg2.ll = arg2;
     farg3.ll = arg3;
-#if USE_PRECISE_EMULATION
+
     if (unlikely(float64_is_signaling_nan(farg1.d) ||
                  float64_is_signaling_nan(farg2.d) ||
                  float64_is_signaling_nan(farg3.d))) {
@@ -1277,10 +1263,7 @@ uint64_t helper_fmadd (uint64_t arg1, uint64_t arg2, uint64_t arg3)
         farg1.d = (farg1.d * farg2.d) + farg3.d;
 #endif
     }
-#else
-    farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
-    farg1.d = float64_add(farg1.d, farg3.d, &env->fp_status);
-#endif
+
     return farg1.ll;
 }
 
@@ -1292,7 +1275,7 @@ uint64_t helper_fmsub (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg1.ll = arg1;
     farg2.ll = arg2;
     farg3.ll = arg3;
-#if USE_PRECISE_EMULATION
+
     if (unlikely(float64_is_signaling_nan(farg1.d) ||
                  float64_is_signaling_nan(farg2.d) ||
                  float64_is_signaling_nan(farg3.d))) {
@@ -1324,10 +1307,6 @@ uint64_t helper_fmsub (uint64_t arg1, uint64_t arg2, uint64_t arg3)
         farg1.d = (farg1.d * farg2.d) - farg3.d;
 #endif
     }
-#else
-    farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
-    farg1.d = float64_sub(farg1.d, farg3.d, &env->fp_status);
-#endif
     return farg1.ll;
 }
 
@@ -1350,7 +1329,6 @@ uint64_t helper_fnmadd (uint64_t arg1, uint64_t arg2, uint64_t arg3)
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
-#if USE_PRECISE_EMULATION
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1371,10 +1349,6 @@ uint64_t helper_fnmadd (uint64_t arg1, uint64_t arg2, uint64_t arg3)
         /* This is OK on x86 hosts */
         farg1.d = (farg1.d * farg2.d) + farg3.d;
 #endif
-#else
-        farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
-        farg1.d = float64_add(farg1.d, farg3.d, &env->fp_status);
-#endif
         if (likely(!float64_is_quiet_nan(farg1.d)))
             farg1.d = float64_chs(farg1.d);
     }
@@ -1400,7 +1374,6 @@ uint64_t helper_fnmsub (uint64_t arg1, uint64_t arg2, uint64_t arg3)
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
-#if USE_PRECISE_EMULATION
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1421,10 +1394,6 @@ uint64_t helper_fnmsub (uint64_t arg1, uint64_t arg2, uint64_t arg3)
         /* This is OK on x86 hosts */
         farg1.d = (farg1.d * farg2.d) - farg3.d;
 #endif
-#else
-        farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
-        farg1.d = float64_sub(farg1.d, farg3.d, &env->fp_status);
-#endif
         if (likely(!float64_is_quiet_nan(farg1.d)))
             farg1.d = float64_chs(farg1.d);
     }
@@ -1438,7 +1407,6 @@ uint64_t helper_frsp (uint64_t arg)
     float32 f32;
     farg.ll = arg;
 
-#if USE_PRECISE_EMULATION
     if (unlikely(float64_is_signaling_nan(farg.d))) {
         /* sNaN square root */
        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
@@ -1446,10 +1414,6 @@ uint64_t helper_frsp (uint64_t arg)
        f32 = float64_to_float32(farg.d, &env->fp_status);
        farg.d = float32_to_float64(f32, &env->fp_status);
     }
-#else
-    f32 = float64_to_float32(farg.d, &env->fp_status);
-    farg.d = float32_to_float64(f32, &env->fp_status);
-#endif
     return farg.ll;
 }
 
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 1/3] target-ppc: remove PRECISE_EMULATION define

[Alexander Graf]

On 02.01.2011, at 15:39, Aurelien Jarno wrote:

> The PRECISE_EMULATION is "hardcoded" to one in target-ppc/exec.h and not
> something easily tunable. Remove it and non-precise emulation code as
> it doesn't make a noticeable difference in speed. People wanting speed
> improvement should use softfloat-native instead.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Looks good from a remote view. Again, I'm not an FP expert :)

Acked-by: Alexander Graf <agraf@suse.de>

Alex

[Qemu-devel] [PATCH 2/3] target-ppc: fix default qNaN

[Aurelien Jarno]

On PPC the default qNaN doesn't have the sign bit set.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/op_helper.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 858877e..279f345 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -643,7 +643,7 @@ static inline uint64_t fload_invalid_op_excp(int op)
         env->fpscr &= ~((1 << FPSCR_FR) | (1 << FPSCR_FI));
         if (ve == 0) {
             /* Set the result to quiet NaN */
-            ret = 0xFFF8000000000000ULL;
+            ret = 0x7FF8000000000000ULL;
             env->fpscr &= ~(0xF << FPSCR_FPCC);
             env->fpscr |= 0x11 << FPSCR_FPCC;
         }
@@ -654,7 +654,7 @@ static inline uint64_t fload_invalid_op_excp(int op)
         env->fpscr &= ~((1 << FPSCR_FR) | (1 << FPSCR_FI));
         if (ve == 0) {
             /* Set the result to quiet NaN */
-            ret = 0xFFF8000000000000ULL;
+            ret = 0x7FF8000000000000ULL;
             env->fpscr &= ~(0xF << FPSCR_FPCC);
             env->fpscr |= 0x11 << FPSCR_FPCC;
         }
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 2/3] target-ppc: fix default qNaN

[Alexander Graf]

On 02.01.2011, at 15:39, Aurelien Jarno wrote:

> On PPC the default qNaN doesn't have the sign bit set.

The spec says "don't care" for the sign bit. Did you extract the value empirically? I'm not saying it's wrong - the default 32 Bit value seems to be 0x7FC0_0000 (2.06 ISA 6.6.2.2).

Hrm ... reading section 5.4.2:

A special QNaN is sometimes supplied as the default QNaN for a disabled invalid-operation exception; it has a plus sign, the leftmost 6 bits of the combination field set to 0b111110 and remaining bits in the combination field and the trailing significand field set to zero.

So I guess you're right.


Acked-by: Alexander Graf <agraf@suse.de>


Alex

[Qemu-devel] Re: [PATCH 2/3] target-ppc: fix default qNaN

[Aurelien Jarno]

On Wed, Jan 05, 2011 at 06:09:34PM +0100, Alexander Graf wrote:
> 
> On 02.01.2011, at 15:39, Aurelien Jarno wrote:
> 
> > On PPC the default qNaN doesn't have the sign bit set.
> 
> The spec says "don't care" for the sign bit. Did you extract the value empirically? I'm not saying it's wrong - the default 32 Bit value seems to be 0x7FC0_0000 (2.06 ISA 6.6.2.2).

"don't care" is for detecting a qNaN which can be represented by
thousands of different values. It's different from the default qNaN.

2.06 ISA 6.6.2.2 is for vector operations, which are always 32-bit. We 
are also using this value, this time through softfloat-specialize.h 
(look at float32_default_nan).

For 64-bit values, the default value is defined in 4.3. For the long
term, we should remove the default qNaN value from
target-ppc/op_helper.c and only use the one in softfloat-specialize.h.
However it needs reworking of part of the softfloat library first.

> Hrm ... reading section 5.4.2:
> 
> A special QNaN is sometimes supplied as the default QNaN for a disabled invalid-operation exception; it has a plus sign, the leftmost 6 bits of the combination field set to 0b111110 and remaining bits in the combination field and the trailing significand field set to zero.
> 

That's for decimal floating point, and not binary floating point,
however it seems they use the same convention.

Thanks for the review.

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

[Qemu-devel] [PATCH 3/3] target-ppc: fix sNaN propagation

[Aurelien Jarno]

The current FPU code returns 0.0 if one of the operand is a
signaling NaN and the VXSNAN exception is disabled.

fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
exception as the operand should be propagated instead of a new
qNaN to be generated. Fix that by calling fload_invalid_op_excp()
only for the exception generation (if enabled), and use the softfloat
code to correctly compute the result.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/op_helper.c |  145 +++++++++++++++++++++++++----------------------
 1 files changed, 77 insertions(+), 68 deletions(-)

diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 279f345..ea030d0 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -975,15 +975,16 @@ uint64_t helper_fadd (uint64_t arg1, uint64_t arg2)
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN addition */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
-                      float64_is_neg(farg1.d) != float64_is_neg(farg2.d))) {
+    if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
+                 float64_is_neg(farg1.d) != float64_is_neg(farg2.d))) {
         /* Magnitude subtraction of infinities */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXISI);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d))) {
+            /* sNaN addition */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg1.d = float64_add(farg1.d, farg2.d, &env->fp_status);
     }
 
@@ -998,15 +999,16 @@ uint64_t helper_fsub (uint64_t arg1, uint64_t arg2)
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN subtraction */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
-                      float64_is_neg(farg1.d) == float64_is_neg(farg2.d))) {
+    if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
+                 float64_is_neg(farg1.d) == float64_is_neg(farg2.d))) {
         /* Magnitude subtraction of infinities */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXISI);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d))) {
+            /* sNaN subtraction */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg1.d = float64_sub(farg1.d, farg2.d, &env->fp_status);
     }
 
@@ -1021,16 +1023,17 @@ uint64_t helper_fmul (uint64_t arg1, uint64_t arg2)
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN multiplication */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
-                        (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
+    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
+                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
-        farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d))) {
+            /* sNaN multiplication */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
+        float64_mul(farg1.d, farg2.d, &env->fp_status);
     }
 
     return farg1.ll;
@@ -1044,17 +1047,18 @@ uint64_t helper_fdiv (uint64_t arg1, uint64_t arg2)
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN division */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d))) {
+    if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d))) {
         /* Division of infinity by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIDI);
     } else if (unlikely(float64_is_zero(farg1.d) && float64_is_zero(farg2.d))) {
         /* Division of zero by zero */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXZDZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d))) {
+            /* sNaN division */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);
     }
 
@@ -1232,16 +1236,17 @@ uint64_t helper_fmadd (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg2.ll = arg2;
     farg3.ll = arg3;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d) ||
-                 float64_is_signaling_nan(farg3.d))) {
-        /* sNaN operation */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
-                        (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
+    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
+                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d) ||
+                     float64_is_signaling_nan(farg3.d))) {
+            /* sNaN operation */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1276,16 +1281,17 @@ uint64_t helper_fmsub (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg2.ll = arg2;
     farg3.ll = arg3;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d) ||
-                 float64_is_signaling_nan(farg3.d))) {
-        /* sNaN operation */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
+    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
                         (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d) ||
+                     float64_is_signaling_nan(farg3.d))) {
+            /* sNaN operation */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1319,16 +1325,17 @@ uint64_t helper_fnmadd (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg2.ll = arg2;
     farg3.ll = arg3;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d) ||
-                 float64_is_signaling_nan(farg3.d))) {
-        /* sNaN operation */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
-                        (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
+    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
+                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d) ||
+                     float64_is_signaling_nan(farg3.d))) {
+            /* sNaN operation */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1364,16 +1371,17 @@ uint64_t helper_fnmsub (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg2.ll = arg2;
     farg3.ll = arg3;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d) ||
-                 float64_is_signaling_nan(farg3.d))) {
-        /* sNaN operation */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
+    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
                         (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d) ||
+                     float64_is_signaling_nan(farg3.d))) {
+            /* sNaN operation */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1410,10 +1418,10 @@ uint64_t helper_frsp (uint64_t arg)
     if (unlikely(float64_is_signaling_nan(farg.d))) {
         /* sNaN square root */
        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else {
-       f32 = float64_to_float32(farg.d, &env->fp_status);
-       farg.d = float32_to_float64(f32, &env->fp_status);
     }
+    f32 = float64_to_float32(farg.d, &env->fp_status);
+    farg.d = float32_to_float64(f32, &env->fp_status);
+
     return farg.ll;
 }
 
@@ -1423,13 +1431,14 @@ uint64_t helper_fsqrt (uint64_t arg)
     CPU_DoubleU farg;
     farg.ll = arg;
 
-    if (unlikely(float64_is_signaling_nan(farg.d))) {
-        /* sNaN square root */
-        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
+    if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
         /* Square root of a negative nonzero number */
         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSQRT);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg.d))) {
+            /* sNaN square root */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg.d = float64_sqrt(farg.d, &env->fp_status);
     }
     return farg.ll;
@@ -1443,10 +1452,9 @@ uint64_t helper_fre (uint64_t arg)
 
     if (unlikely(float64_is_signaling_nan(farg.d))) {
         /* sNaN reciprocal */
-        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else {
-        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
+        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
     }
+    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
     return farg.d;
 }
 
@@ -1460,11 +1468,11 @@ uint64_t helper_fres (uint64_t arg)
     if (unlikely(float64_is_signaling_nan(farg.d))) {
         /* sNaN reciprocal */
         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else {
-        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
-        f32 = float64_to_float32(farg.d, &env->fp_status);
-        farg.d = float32_to_float64(f32, &env->fp_status);
     }
+    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
+    f32 = float64_to_float32(farg.d, &env->fp_status);
+    farg.d = float32_to_float64(f32, &env->fp_status);
+
     return farg.ll;
 }
 
@@ -1475,13 +1483,14 @@ uint64_t helper_frsqrte (uint64_t arg)
     float32 f32;
     farg.ll = arg;
 
-    if (unlikely(float64_is_signaling_nan(farg.d))) {
-        /* sNaN reciprocal square root */
-        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
+    if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
         /* Reciprocal square root of a negative nonzero number */
         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSQRT);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg.d))) {
+            /* sNaN reciprocal square root */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg.d = float64_sqrt(farg.d, &env->fp_status);
         farg.d = float64_div(float64_one, farg.d, &env->fp_status);
         f32 = float64_to_float32(farg.d, &env->fp_status);
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 3/3] target-ppc: fix sNaN propagation

[Alexander Graf]

On 02.01.2011, at 15:39, Aurelien Jarno wrote:

> The current FPU code returns 0.0 if one of the operand is a
> signaling NaN and the VXSNAN exception is disabled.
> 
> fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> exception as the operand should be propagated instead of a new
> qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> only for the exception generation (if enabled), and use the softfloat
> code to correctly compute the result.

Reading through this I'm afraid I understand too little of the matter. Anyone else who's more proficient in FP feels like taking up the review?


Alex

[Qemu-devel] Re: [PATCH 3/3] target-ppc: fix sNaN propagation

[Aurelien Jarno]

On Wed, Jan 05, 2011 at 06:20:07PM +0100, Alexander Graf wrote:
> 
> On 02.01.2011, at 15:39, Aurelien Jarno wrote:
> 
> > The current FPU code returns 0.0 if one of the operand is a
> > signaling NaN and the VXSNAN exception is disabled.
> > 
> > fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> > exception as the operand should be propagated instead of a new
> > qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> > only for the exception generation (if enabled), and use the softfloat
> > code to correctly compute the result.
> 
> Reading through this I'm afraid I understand too little of the matter. Anyone else who's more proficient in FP feels like taking up the review?
> 

Anybody to review this code? Nathan maybe?

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

Re: [Qemu-devel] [PATCH 3/3] target-ppc: fix sNaN propagation

[Peter Maydell]

On 2 January 2011 08:39, Aurelien Jarno <aurelien@aurel32.net> wrote:
> The current FPU code returns 0.0 if one of the operand is a
> signaling NaN and the VXSNAN exception is disabled.
>
> fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> exception as the operand should be propagated instead of a new
> qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> only for the exception generation (if enabled), and use the softfloat
> code to correctly compute the result.
>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

> @@ -1410,10 +1418,10 @@ uint64_t helper_frsp (uint64_t arg)
>     if (unlikely(float64_is_signaling_nan(farg.d))) {
>         /* sNaN square root */
>        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> -    } else {
> -       f32 = float64_to_float32(farg.d, &env->fp_status);
> -       farg.d = float32_to_float64(f32, &env->fp_status);
>     }
> +    f32 = float64_to_float32(farg.d, &env->fp_status);
> +    farg.d = float32_to_float64(f32, &env->fp_status);
> +
>     return farg.ll;
>  }

Most of these changes are to ignoring the result from
fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN),
but this one leaves it assigning a value to farg.ll -- is there
any reason for that? (It looks like the assignment gets
immediately overwritten by the assignment to farg.d later.)

> @@ -1460,11 +1468,11 @@ uint64_t helper_fres (uint64_t arg)
>     if (unlikely(float64_is_signaling_nan(farg.d))) {
>         /* sNaN reciprocal */
>         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> -    } else {
> -        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> -        f32 = float64_to_float32(farg.d, &env->fp_status);
> -        farg.d = float32_to_float64(f32, &env->fp_status);
>     }
> +    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> +    f32 = float64_to_float32(farg.d, &env->fp_status);
> +    farg.d = float32_to_float64(f32, &env->fp_status);
> +
>     return farg.ll;
>  }
>

Ditto for this hunk.

Looks plausible to me other than that.

-- PMM

Re: [Qemu-devel] [PATCH 3/3] target-ppc: fix sNaN propagation

[Aurelien Jarno]

On Mon, Jan 10, 2011 at 06:14:18PM -0600, Peter Maydell wrote:
> On 2 January 2011 08:39, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > The current FPU code returns 0.0 if one of the operand is a
> > signaling NaN and the VXSNAN exception is disabled.
> >
> > fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> > exception as the operand should be propagated instead of a new
> > qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> > only for the exception generation (if enabled), and use the softfloat
> > code to correctly compute the result.
> >
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> > @@ -1410,10 +1418,10 @@ uint64_t helper_frsp (uint64_t arg)
> >     if (unlikely(float64_is_signaling_nan(farg.d))) {
> >         /* sNaN square root */
> >        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > -    } else {
> > -       f32 = float64_to_float32(farg.d, &env->fp_status);
> > -       farg.d = float32_to_float64(f32, &env->fp_status);
> >     }
> > +    f32 = float64_to_float32(farg.d, &env->fp_status);
> > +    farg.d = float32_to_float64(f32, &env->fp_status);
> > +
> >     return farg.ll;
> >  }
> 
> Most of these changes are to ignoring the result from
> fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN),
> but this one leaves it assigning a value to farg.ll -- is there
> any reason for that? (It looks like the assignment gets
> immediately overwritten by the assignment to farg.d later.)

It is actually a mistake, though the result is correct. I'll resend the
patch after fixing that, thanks for the review.

> > @@ -1460,11 +1468,11 @@ uint64_t helper_fres (uint64_t arg)
> >     if (unlikely(float64_is_signaling_nan(farg.d))) {
> >         /* sNaN reciprocal */
> >         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > -    } else {
> > -        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> > -        f32 = float64_to_float32(farg.d, &env->fp_status);
> > -        farg.d = float32_to_float64(f32, &env->fp_status);
> >     }
> > +    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> > +    f32 = float64_to_float32(farg.d, &env->fp_status);
> > +    farg.d = float32_to_float64(f32, &env->fp_status);
> > +
> >     return farg.ll;
> >  }
> >
> 
> Ditto for this hunk.
> 
> Looks plausible to me other than that.
> 

Same here.

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