[Qemu-devel] [PATCH 0/7] target-ppc: Assorted Floating Point Bugs and Cleanup

[Qemu-devel] [PATCH 0/7] target-ppc: Assorted Floating Point Bugs and Cleanup

[Tom Musta]

This patch series corrects some issues with floating point emulation
on Power.

Patch 1 corrects a corner case in the square root instructions, which
incorrectly react to NaNs whose sign bit is a 1.

Patches 2-6 correct a pervasive problem with modeling of the CR[1]
field (i.e. the so-called "dot form" instructions of the FPU).

The bugs were found by running random test patterns through actual Power
hardware (P7 and P8) and comparing against QEMU.

The patches conflict quite a bit with Paolo's series that splits CR into
32 one bit registers.  Paolo: is V3 of your patch series coming anytime 
soon?

Tom Musta (7):
  target-ppc: VXSQRT Should Not Be Set for NaNs
  target-ppc: Introduce gen_set_cr1_from_fpscr
  target-ppc: Fix Floating Point Move Instructions That Set CR1
  target-ppc: mffs. Should Set CR1 from FPSCR Bits
  target-ppc: Fully Migrate to gen_set_cr1_from_fpscr
  target-ppc: Eliminate set_fprf Argument From gen_compute_fprf
  target-ppc: Eliminate set_fprf Argument From helper_compute_fprf

 target-ppc/fpu_helper.c |   85 +++++++++++++++++++-------------------
 target-ppc/helper.h     |    2 +-
 target-ppc/translate.c  |  105 +++++++++++++++++++++++++++++++----------------
 3 files changed, 113 insertions(+), 79 deletions(-)

[Qemu-devel] [PATCH 1/7] target-ppc: VXSQRT Should Not Be Set for NaNs

[Tom Musta]

The Power ISA square root instructions (fsqrt[s], frsqrte[s]) must
set the FPSCR[VXSQRT] flag when operating on a negative value.
However, NaNs have no sign and therefore this flag should not
be set when operating on one.

Change the order of the checks in the helper code.  Move the
SNaN-to-QNaN macro to the top of the file so that it can be
re-used.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
 target-ppc/fpu_helper.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
index da93d12..288401d 100644
--- a/target-ppc/fpu_helper.c
+++ b/target-ppc/fpu_helper.c
@@ -19,6 +19,9 @@
 #include "cpu.h"
 #include "exec/helper-proto.h"
 
+#define float64_snan_to_qnan(x) ((x) | 0x0008000000000000ULL)
+#define float32_snan_to_qnan(x) ((x) | 0x00400000)
+
 /*****************************************************************************/
 /* Floating point operations helpers */
 uint64_t helper_float32_to_float64(CPUPPCState *env, uint32_t arg)
@@ -926,14 +929,16 @@ uint64_t helper_fsqrt(CPUPPCState *env, uint64_t arg)
 
     farg.ll = arg;
 
-    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
-    } else {
+    if (unlikely(float64_is_any_nan(farg.d))) {
         if (unlikely(float64_is_signaling_nan(farg.d))) {
-            /* sNaN square root */
+            /* sNaN reciprocal square root */
             fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+            farg.ll = float64_snan_to_qnan(farg.ll);
         }
+    } else 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(env, POWERPC_EXCP_FP_VXSQRT, 1);
+    } else {
         farg.d = float64_sqrt(farg.d, &env->fp_status);
     }
     return farg.ll;
@@ -980,17 +985,20 @@ uint64_t helper_frsqrte(CPUPPCState *env, uint64_t arg)
 
     farg.ll = arg;
 
-    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
-    } else {
+    if (unlikely(float64_is_any_nan(farg.d))) {
         if (unlikely(float64_is_signaling_nan(farg.d))) {
             /* sNaN reciprocal square root */
             fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+            farg.ll = float64_snan_to_qnan(farg.ll);
         }
+    } else 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(env, POWERPC_EXCP_FP_VXSQRT, 1);
+    } else {
         farg.d = float64_sqrt(farg.d, &env->fp_status);
         farg.d = float64_div(float64_one, farg.d, &env->fp_status);
     }
+
     return farg.ll;
 }
 
@@ -2388,9 +2396,6 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                      \
 VSX_SCALAR_CMP(xscmpodp, 1)
 VSX_SCALAR_CMP(xscmpudp, 0)
 
-#define float64_snan_to_qnan(x) ((x) | 0x0008000000000000ULL)
-#define float32_snan_to_qnan(x) ((x) | 0x00400000)
-
 /* VSX_MAX_MIN - VSX floating point maximum/minimum
  *   name  - instruction mnemonic
  *   op    - operation (max or min)
-- 
1.7.1

[Qemu-devel] [PATCH 2/7] target-ppc: Introduce gen_set_cr1_from_fpscr

[Tom Musta]

The Power ISA supports a mode in many floating point instructions whereby
the Condition Register field 1 (CR[1]) receives a copy of the Floating
Point Status (FPSCR) bits 32:35, also known as FX, FEX VX and OX.

The existing QEMU code is mostly wrong -- CR[1] is set to the Floating
Point Condition Code (FPSCR[FPCC]).  Furthermore, this code is buried
inside the code that generates the FPSCR[FPRF] code, which is awkward.

Introduce a new generator utility that correctly sets CR[1] from the
FPSCR bits.  Subsequent patches will correct various segments of
the defective code and will clean up the gen_compute_fprf()
utility.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
 target-ppc/translate.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index d03daea..7775bf4 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -249,6 +249,14 @@ static inline void gen_reset_fpstatus(void)
     gen_helper_reset_fpstatus(cpu_env);
 }
 
+static inline void gen_set_cr1_from_fpscr(void)
+{
+    TCGv_i32 t0 = tcg_temp_new_i32();
+    tcg_gen_trunc_tl_i32(t0, cpu_fpscr);
+    tcg_gen_shri_i32(cpu_crf[1], t0, 28);
+    tcg_temp_free_i32(t0);
+}
+
 static inline void gen_compute_fprf(TCGv_i64 arg, int set_fprf, int set_rc)
 {
     TCGv_i32 t0 = tcg_temp_new_i32();
-- 
1.7.1

[Qemu-devel] [PATCH 3/7] target-ppc: Fix Floating Point Move Instructions That Set CR1

[Tom Musta]

The Floating Point Move instructions (fmr., fabs., fnabs., fneg.,
and fcpsgn.) incorrectly copy FPSCR[FPCC] instead of [FX,FEX,VX,OX].
Furthermore, the current code does this via a call to gen_compute_fprf,
which is awkward since these instructions do not actually set FPRF.

Change the code to use the newly added gen_set_cr1_from_fpscr
utility.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
 target-ppc/translate.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 7775bf4..9653ba9 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -2393,7 +2393,9 @@ static void gen_fabs(DisasContext *ctx)
     }
     tcg_gen_andi_i64(cpu_fpr[rD(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
                      ~(1ULL << 63));
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 0, Rc(ctx->opcode) != 0);
+    if (unlikely(Rc(ctx->opcode))) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 /* fmr  - fmr. */
@@ -2405,7 +2407,9 @@ static void gen_fmr(DisasContext *ctx)
         return;
     }
     tcg_gen_mov_i64(cpu_fpr[rD(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 0, Rc(ctx->opcode) != 0);
+    if (unlikely(Rc(ctx->opcode))) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 /* fnabs */
@@ -2418,7 +2422,9 @@ static void gen_fnabs(DisasContext *ctx)
     }
     tcg_gen_ori_i64(cpu_fpr[rD(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
                     1ULL << 63);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 0, Rc(ctx->opcode) != 0);
+    if (unlikely(Rc(ctx->opcode))) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 /* fneg */
@@ -2431,7 +2437,9 @@ static void gen_fneg(DisasContext *ctx)
     }
     tcg_gen_xori_i64(cpu_fpr[rD(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
                      1ULL << 63);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 0, Rc(ctx->opcode) != 0);
+    if (unlikely(Rc(ctx->opcode))) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 /* fcpsgn: PowerPC 2.05 specification */
@@ -2444,7 +2452,9 @@ static void gen_fcpsgn(DisasContext *ctx)
     }
     tcg_gen_deposit_i64(cpu_fpr[rD(ctx->opcode)], cpu_fpr[rA(ctx->opcode)],
                         cpu_fpr[rB(ctx->opcode)], 0, 63);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 0, Rc(ctx->opcode) != 0);
+    if (unlikely(Rc(ctx->opcode))) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 static void gen_fmrgew(DisasContext *ctx)
-- 
1.7.1

[Qemu-devel] [PATCH 4/7] target-ppc: mffs. Should Set CR1 from FPSCR Bits

[Tom Musta]

Update the Move From FPSCR (mffs.) instruction to correctly
set CR[1] from FPSCR[FX,FEX,VX,OX].

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
 target-ppc/translate.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 9653ba9..0247af5 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -2512,7 +2512,9 @@ static void gen_mffs(DisasContext *ctx)
     }
     gen_reset_fpstatus();
     tcg_gen_extu_tl_i64(cpu_fpr[rD(ctx->opcode)], cpu_fpscr);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 0, Rc(ctx->opcode) != 0);
+    if (unlikely(Rc(ctx->opcode))) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 /* mtfsb0 */
-- 
1.7.1

[Qemu-devel] [PATCH 5/7] target-ppc: Fully Migrate to gen_set_cr1_from_fpscr

[Tom Musta]

Eliminate the set_rc argument from the gen_compute_fprf utility and
the corresponding (and incorrect) implementation.  Replace it with
calls to the gen_set_cr1_from_fpscr() utility.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
 target-ppc/translate.c |   55 ++++++++++++++++++++++++++++-------------------
 1 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 0247af5..d719cdf 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -257,7 +257,7 @@ static inline void gen_set_cr1_from_fpscr(void)
     tcg_temp_free_i32(t0);
 }
 
-static inline void gen_compute_fprf(TCGv_i64 arg, int set_fprf, int set_rc)
+static inline void gen_compute_fprf(TCGv_i64 arg, int set_fprf)
 {
     TCGv_i32 t0 = tcg_temp_new_i32();
 
@@ -265,15 +265,7 @@ static inline void gen_compute_fprf(TCGv_i64 arg, int set_fprf, int set_rc)
         /* This case might be optimized later */
         tcg_gen_movi_i32(t0, 1);
         gen_helper_compute_fprf(t0, cpu_env, arg, t0);
-        if (unlikely(set_rc)) {
-            tcg_gen_mov_i32(cpu_crf[1], t0);
-        }
         gen_helper_float_check_status(cpu_env);
-    } else if (unlikely(set_rc)) {
-        /* We always need to compute fpcc */
-        tcg_gen_movi_i32(t0, 0);
-        gen_helper_compute_fprf(t0, cpu_env, arg, t0);
-        tcg_gen_mov_i32(cpu_crf[1], t0);
     }
 
     tcg_temp_free_i32(t0);
@@ -2116,8 +2108,10 @@ static void gen_f##name(DisasContext *ctx)                                    \
         gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                    \
                         cpu_fpr[rD(ctx->opcode)]);                            \
     }                                                                         \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf,                      \
-                     Rc(ctx->opcode) != 0);                                   \
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
+        gen_set_cr1_from_fpscr();                                             \
+    }                                                                         \
 }
 
 #define GEN_FLOAT_ACB(name, op2, set_fprf, type)                              \
@@ -2141,8 +2135,10 @@ static void gen_f##name(DisasContext *ctx)                                    \
         gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                    \
                         cpu_fpr[rD(ctx->opcode)]);                            \
     }                                                                         \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)],                                \
-                     set_fprf, Rc(ctx->opcode) != 0);                         \
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
+        gen_set_cr1_from_fpscr();                                             \
+    }                                                                         \
 }
 #define GEN_FLOAT_AB(name, op2, inval, set_fprf, type)                        \
 _GEN_FLOAT_AB(name, name, 0x3F, op2, inval, 0, set_fprf, type);               \
@@ -2165,8 +2161,10 @@ static void gen_f##name(DisasContext *ctx)                                    \
         gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                    \
                         cpu_fpr[rD(ctx->opcode)]);                            \
     }                                                                         \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)],                                \
-                     set_fprf, Rc(ctx->opcode) != 0);                         \
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
+        gen_set_cr1_from_fpscr();                                             \
+    }                                                                         \
 }
 #define GEN_FLOAT_AC(name, op2, inval, set_fprf, type)                        \
 _GEN_FLOAT_AC(name, name, 0x3F, op2, inval, 0, set_fprf, type);               \
@@ -2184,8 +2182,10 @@ static void gen_f##name(DisasContext *ctx)                                    \
     gen_reset_fpstatus();                                                     \
     gen_helper_f##name(cpu_fpr[rD(ctx->opcode)], cpu_env,                     \
                        cpu_fpr[rB(ctx->opcode)]);                             \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)],                                \
-                     set_fprf, Rc(ctx->opcode) != 0);                         \
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
+        gen_set_cr1_from_fpscr();                                             \
+    }                                                                         \
 }
 
 #define GEN_FLOAT_BS(name, op1, op2, set_fprf, type)                          \
@@ -2200,8 +2200,10 @@ static void gen_f##name(DisasContext *ctx)                                    \
     gen_reset_fpstatus();                                                     \
     gen_helper_f##name(cpu_fpr[rD(ctx->opcode)], cpu_env,                     \
                        cpu_fpr[rB(ctx->opcode)]);                             \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)],                                \
-                     set_fprf, Rc(ctx->opcode) != 0);                         \
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
+        gen_set_cr1_from_fpscr();                                             \
+    }                                                                         \
 }
 
 /* fadd - fadds */
@@ -2234,7 +2236,10 @@ static void gen_frsqrtes(DisasContext *ctx)
                        cpu_fpr[rB(ctx->opcode)]);
     gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,
                     cpu_fpr[rD(ctx->opcode)]);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1, Rc(ctx->opcode) != 0);
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1);
+    if (unlikely(Rc(ctx->opcode) != 0)) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 /* fsel */
@@ -2255,7 +2260,10 @@ static void gen_fsqrt(DisasContext *ctx)
     gen_reset_fpstatus();
     gen_helper_fsqrt(cpu_fpr[rD(ctx->opcode)], cpu_env,
                      cpu_fpr[rB(ctx->opcode)]);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1, Rc(ctx->opcode) != 0);
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1);
+    if (unlikely(Rc(ctx->opcode) != 0)) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 static void gen_fsqrts(DisasContext *ctx)
@@ -2271,7 +2279,10 @@ static void gen_fsqrts(DisasContext *ctx)
                      cpu_fpr[rB(ctx->opcode)]);
     gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,
                     cpu_fpr[rD(ctx->opcode)]);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1, Rc(ctx->opcode) != 0);
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1);
+    if (unlikely(Rc(ctx->opcode) != 0)) {
+        gen_set_cr1_from_fpscr();
+    }
 }
 
 /***                     Floating-Point multiply-and-add                   ***/
-- 
1.7.1

[Qemu-devel] [PATCH 6/7] target-ppc: Eliminate set_fprf Argument From gen_compute_fprf

[Tom Musta]

The set_fprf argument to the gen_compute_fprf() utility is no longer
needed -- gen_compute_fprf() is now called only when FPRF is actually
computed and set.  Eliminate the obsolete argument.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
 target-ppc/translate.c |   38 +++++++++++++++++++++++---------------
 1 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index d719cdf..c039494 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -257,16 +257,14 @@ static inline void gen_set_cr1_from_fpscr(void)
     tcg_temp_free_i32(t0);
 }
 
-static inline void gen_compute_fprf(TCGv_i64 arg, int set_fprf)
+static inline void gen_compute_fprf(TCGv_i64 arg)
 {
     TCGv_i32 t0 = tcg_temp_new_i32();
 
-    if (set_fprf != 0) {
-        /* This case might be optimized later */
-        tcg_gen_movi_i32(t0, 1);
-        gen_helper_compute_fprf(t0, cpu_env, arg, t0);
-        gen_helper_float_check_status(cpu_env);
-    }
+    /* This case might be optimized later */
+    tcg_gen_movi_i32(t0, 1);
+    gen_helper_compute_fprf(t0, cpu_env, arg, t0);
+    gen_helper_float_check_status(cpu_env);
 
     tcg_temp_free_i32(t0);
 }
@@ -2108,7 +2106,9 @@ static void gen_f##name(DisasContext *ctx)                                    \
         gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                    \
                         cpu_fpr[rD(ctx->opcode)]);                            \
     }                                                                         \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (set_fprf) {                                                           \
+        gen_compute_fprf(cpu_fpr[rD(ctx->opcode)]);                           \
+    }                                                                         \
     if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
         gen_set_cr1_from_fpscr();                                             \
     }                                                                         \
@@ -2135,7 +2135,9 @@ static void gen_f##name(DisasContext *ctx)                                    \
         gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                    \
                         cpu_fpr[rD(ctx->opcode)]);                            \
     }                                                                         \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (set_fprf) {                                                           \
+        gen_compute_fprf(cpu_fpr[rD(ctx->opcode)]);                           \
+    }                                                                         \
     if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
         gen_set_cr1_from_fpscr();                                             \
     }                                                                         \
@@ -2161,7 +2163,9 @@ static void gen_f##name(DisasContext *ctx)                                    \
         gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                    \
                         cpu_fpr[rD(ctx->opcode)]);                            \
     }                                                                         \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (set_fprf) {                                                           \
+        gen_compute_fprf(cpu_fpr[rD(ctx->opcode)]);                           \
+    }                                                                         \
     if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
         gen_set_cr1_from_fpscr();                                             \
     }                                                                         \
@@ -2182,7 +2186,9 @@ static void gen_f##name(DisasContext *ctx)                                    \
     gen_reset_fpstatus();                                                     \
     gen_helper_f##name(cpu_fpr[rD(ctx->opcode)], cpu_env,                     \
                        cpu_fpr[rB(ctx->opcode)]);                             \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (set_fprf) {                                                           \
+        gen_compute_fprf(cpu_fpr[rD(ctx->opcode)]);                           \
+    }                                                                         \
     if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
         gen_set_cr1_from_fpscr();                                             \
     }                                                                         \
@@ -2200,7 +2206,9 @@ static void gen_f##name(DisasContext *ctx)                                    \
     gen_reset_fpstatus();                                                     \
     gen_helper_f##name(cpu_fpr[rD(ctx->opcode)], cpu_env,                     \
                        cpu_fpr[rB(ctx->opcode)]);                             \
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], set_fprf);                     \
+    if (set_fprf) {                                                           \
+        gen_compute_fprf(cpu_fpr[rD(ctx->opcode)]);                           \
+    }                                                                         \
     if (unlikely(Rc(ctx->opcode) != 0)) {                                     \
         gen_set_cr1_from_fpscr();                                             \
     }                                                                         \
@@ -2236,7 +2244,7 @@ static void gen_frsqrtes(DisasContext *ctx)
                        cpu_fpr[rB(ctx->opcode)]);
     gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,
                     cpu_fpr[rD(ctx->opcode)]);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1);
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)]);
     if (unlikely(Rc(ctx->opcode) != 0)) {
         gen_set_cr1_from_fpscr();
     }
@@ -2260,7 +2268,7 @@ static void gen_fsqrt(DisasContext *ctx)
     gen_reset_fpstatus();
     gen_helper_fsqrt(cpu_fpr[rD(ctx->opcode)], cpu_env,
                      cpu_fpr[rB(ctx->opcode)]);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1);
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)]);
     if (unlikely(Rc(ctx->opcode) != 0)) {
         gen_set_cr1_from_fpscr();
     }
@@ -2279,7 +2287,7 @@ static void gen_fsqrts(DisasContext *ctx)
                      cpu_fpr[rB(ctx->opcode)]);
     gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,
                     cpu_fpr[rD(ctx->opcode)]);
-    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 1);
+    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)]);
     if (unlikely(Rc(ctx->opcode) != 0)) {
         gen_set_cr1_from_fpscr();
     }
-- 
1.7.1

[Qemu-devel] [PATCH 7/7] target-ppc: Eliminate set_fprf Argument From helper_compute_fprf

[Tom Musta]

The set_fprf argument to the helper_compute_fprf helper function
is no longer necessary -- the helper is only invoked when FPSCR[FPRF]
is going to be set.

Eliminate the unnecessary argument from the function signature and
its corresponding implementation.  Change the return value of the
helper to "void".  Update the name of the local variable "ret" to
"fprf", which now makes more sense.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---
 target-ppc/fpu_helper.c |   56 +++++++++++++++++++++-------------------------
 target-ppc/helper.h     |    2 +-
 target-ppc/translate.c  |    8 +------
 3 files changed, 28 insertions(+), 38 deletions(-)

diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
index 288401d..34ddda0 100644
--- a/target-ppc/fpu_helper.c
+++ b/target-ppc/fpu_helper.c
@@ -63,59 +63,55 @@ static inline int ppc_float64_get_unbiased_exp(float64 f)
     return ((f >> 52) & 0x7FF) - 1023;
 }
 
-uint32_t helper_compute_fprf(CPUPPCState *env, uint64_t arg, uint32_t set_fprf)
+void helper_compute_fprf(CPUPPCState *env, uint64_t arg)
 {
     CPU_DoubleU farg;
     int isneg;
-    int ret;
+    int fprf;
 
     farg.ll = arg;
     isneg = float64_is_neg(farg.d);
     if (unlikely(float64_is_any_nan(farg.d))) {
         if (float64_is_signaling_nan(farg.d)) {
             /* Signaling NaN: flags are undefined */
-            ret = 0x00;
+            fprf = 0x00;
         } else {
             /* Quiet NaN */
-            ret = 0x11;
+            fprf = 0x11;
         }
     } else if (unlikely(float64_is_infinity(farg.d))) {
         /* +/- infinity */
         if (isneg) {
-            ret = 0x09;
+            fprf = 0x09;
         } else {
-            ret = 0x05;
+            fprf = 0x05;
         }
     } else {
         if (float64_is_zero(farg.d)) {
             /* +/- zero */
             if (isneg) {
-                ret = 0x12;
+                fprf = 0x12;
             } else {
-                ret = 0x02;
+                fprf = 0x02;
             }
         } else {
             if (isden(farg.d)) {
                 /* Denormalized numbers */
-                ret = 0x10;
+                fprf = 0x10;
             } else {
                 /* Normalized numbers */
-                ret = 0x00;
+                fprf = 0x00;
             }
             if (isneg) {
-                ret |= 0x08;
+                fprf |= 0x08;
             } else {
-                ret |= 0x04;
+                fprf |= 0x04;
             }
         }
     }
-    if (set_fprf) {
-        /* We update FPSCR_FPRF */
-        env->fpscr &= ~(0x1F << FPSCR_FPRF);
-        env->fpscr |= ret << FPSCR_FPRF;
-    }
-    /* We just need fpcc to update Rc1 */
-    return ret & 0xF;
+    /* We update FPSCR_FPRF */
+    env->fpscr &= ~(0x1F << FPSCR_FPRF);
+    env->fpscr |= fprf << FPSCR_FPRF;
 }
 
 /* Floating-point invalid operations exception */
@@ -1859,7 +1855,7 @@ void helper_##name(CPUPPCState *env, uint32_t opcode)                        \
         }                                                                    \
                                                                              \
         if (sfprf) {                                                         \
-            helper_compute_fprf(env, xt.fld, sfprf);                         \
+            helper_compute_fprf(env, xt.fld);                                \
         }                                                                    \
     }                                                                        \
     putVSR(xT(opcode), &xt, env);                                            \
@@ -1914,7 +1910,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                          \
         }                                                                    \
                                                                              \
         if (sfprf) {                                                         \
-            helper_compute_fprf(env, xt.fld, sfprf);                         \
+            helper_compute_fprf(env, xt.fld);                                \
         }                                                                    \
     }                                                                        \
                                                                              \
@@ -1968,7 +1964,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                           \
         }                                                                     \
                                                                               \
         if (sfprf) {                                                          \
-            helper_compute_fprf(env, xt.fld, sfprf);                          \
+            helper_compute_fprf(env, xt.fld);                                 \
         }                                                                     \
     }                                                                         \
                                                                               \
@@ -2009,7 +2005,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                           \
         }                                                                     \
                                                                               \
         if (sfprf) {                                                          \
-            helper_compute_fprf(env, xt.fld, sfprf);                          \
+            helper_compute_fprf(env, xt.fld);                                 \
         }                                                                     \
     }                                                                         \
                                                                               \
@@ -2058,7 +2054,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                          \
         }                                                                    \
                                                                              \
         if (sfprf) {                                                         \
-            helper_compute_fprf(env, xt.fld, sfprf);                         \
+            helper_compute_fprf(env, xt.fld);                                \
         }                                                                    \
     }                                                                        \
                                                                              \
@@ -2108,7 +2104,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                          \
         }                                                                    \
                                                                              \
         if (sfprf) {                                                         \
-            helper_compute_fprf(env, xt.fld, sfprf);                         \
+            helper_compute_fprf(env, xt.fld);                                \
         }                                                                    \
     }                                                                        \
                                                                              \
@@ -2308,7 +2304,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                           \
         }                                                                     \
                                                                               \
         if (sfprf) {                                                          \
-            helper_compute_fprf(env, xt_out.fld, sfprf);                      \
+            helper_compute_fprf(env, xt_out.fld);                             \
         }                                                                     \
     }                                                                         \
     putVSR(xT(opcode), &xt_out, env);                                         \
@@ -2515,7 +2511,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                \
         }                                                          \
         if (sfprf) {                                               \
             helper_compute_fprf(env, ttp##_to_float64(xt.tfld,     \
-                                &env->fp_status), sfprf);          \
+                                &env->fp_status));                 \
         }                                                          \
     }                                                              \
                                                                    \
@@ -2625,7 +2621,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                     \
             xt.tfld = helper_frsp(env, xt.tfld);                        \
         }                                                               \
         if (sfprf) {                                                    \
-            helper_compute_fprf(env, xt.tfld, sfprf);                   \
+            helper_compute_fprf(env, xt.tfld);                          \
         }                                                               \
     }                                                                   \
                                                                         \
@@ -2680,7 +2676,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                    \
             xt.fld = tp##_round_to_int(xb.fld, &env->fp_status);       \
         }                                                              \
         if (sfprf) {                                                   \
-            helper_compute_fprf(env, xt.fld, sfprf);                   \
+            helper_compute_fprf(env, xt.fld);                          \
         }                                                              \
     }                                                                  \
                                                                        \
@@ -2720,7 +2716,7 @@ uint64_t helper_xsrsp(CPUPPCState *env, uint64_t xb)
 
     uint64_t xt = helper_frsp(env, xb);
 
-    helper_compute_fprf(env, xt, 1);
+    helper_compute_fprf(env, xt);
     helper_float_check_status(env);
     return xt;
 }
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index 0cfdc8a..758f7d1 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -52,7 +52,7 @@ DEF_HELPER_FLAGS_2(brinc, TCG_CALL_NO_RWG_SE, tl, tl, tl)
 
 DEF_HELPER_1(float_check_status, void, env)
 DEF_HELPER_1(reset_fpstatus, void, env)
-DEF_HELPER_3(compute_fprf, i32, env, i64, i32)
+DEF_HELPER_2(compute_fprf, void, env, i64)
 DEF_HELPER_3(store_fpscr, void, env, i64, i32)
 DEF_HELPER_2(fpscr_clrbit, void, env, i32)
 DEF_HELPER_2(fpscr_setbit, void, env, i32)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index c039494..4a00935 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -259,14 +259,8 @@ static inline void gen_set_cr1_from_fpscr(void)
 
 static inline void gen_compute_fprf(TCGv_i64 arg)
 {
-    TCGv_i32 t0 = tcg_temp_new_i32();
-
-    /* This case might be optimized later */
-    tcg_gen_movi_i32(t0, 1);
-    gen_helper_compute_fprf(t0, cpu_env, arg, t0);
+    gen_helper_compute_fprf(cpu_env, arg);
     gen_helper_float_check_status(cpu_env);
-
-    tcg_temp_free_i32(t0);
 }
 
 static inline void gen_set_access_type(DisasContext *ctx, int access_type)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/7] target-ppc: Assorted Floating Point Bugs and Cleanup

[Paolo Bonzini]

On 03/11/2014 21:01, Tom Musta wrote:
> The patches conflict quite a bit with Paolo's series that splits CR into
> 32 one bit registers.  Paolo: is V3 of your patch series coming anytime 
> soon?

Yes, but no big deal.  I'll rebase on top.

Paolo

Re: [Qemu-devel] [PATCH 2/7] target-ppc: Introduce gen_set_cr1_from_fpscr

[Paolo Bonzini]

What tree are these patches based on?  Alex's tree already has a

commit 15a6b218c221a34b12e81790f427efec3108dce9
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Thu Aug 28 19:15:07 2014 +0200

    ppc: rename gen_set_cr6_from_fpscr

    It sets CR1, not CR6 (and the spec agrees).

    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Reviewed-by: Tom Musta <tommusta@gmail.com>
    Tested-by: Tom Musta <tommusta@gmail.com>
    Signed-off-by: Alexander Graf <agraf@suse.de>

that conflicts (semantically) with this.

Paolo

On 03/11/2014 21:01, Tom Musta wrote:
> The Power ISA supports a mode in many floating point instructions whereby
> the Condition Register field 1 (CR[1]) receives a copy of the Floating
> Point Status (FPSCR) bits 32:35, also known as FX, FEX VX and OX.
> 
> The existing QEMU code is mostly wrong -- CR[1] is set to the Floating
> Point Condition Code (FPSCR[FPCC]).  Furthermore, this code is buried
> inside the code that generates the FPSCR[FPRF] code, which is awkward.
> 
> Introduce a new generator utility that correctly sets CR[1] from the
> FPSCR bits.  Subsequent patches will correct various segments of
> the defective code and will clean up the gen_compute_fprf()
> utility.
> 
> Signed-off-by: Tom Musta <tommusta@gmail.com>
> ---
>  target-ppc/translate.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index d03daea..7775bf4 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -249,6 +249,14 @@ static inline void gen_reset_fpstatus(void)
>      gen_helper_reset_fpstatus(cpu_env);
>  }
>  
> +static inline void gen_set_cr1_from_fpscr(void)
> +{
> +    TCGv_i32 t0 = tcg_temp_new_i32();
> +    tcg_gen_trunc_tl_i32(t0, cpu_fpscr);
> +    tcg_gen_shri_i32(cpu_crf[1], t0, 28);
> +    tcg_temp_free_i32(t0);
> +}
> +
>  static inline void gen_compute_fprf(TCGv_i64 arg, int set_fprf, int set_rc)
>  {
>      TCGv_i32 t0 = tcg_temp_new_i32();
> 

Re: [Qemu-devel] [PATCH 2/7] target-ppc: Introduce gen_set_cr1_from_fpscr

[Alexander Graf]

> Am 04.11.2014 um 16:58 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> What tree are these patches based on?  Alex's tree already has a
> 
> commit 15a6b218c221a34b12e81790f427efec3108dce9
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Thu Aug 28 19:15:07 2014 +0200
> 
>    ppc: rename gen_set_cr6_from_fpscr
> 
>    It sets CR1, not CR6 (and the spec agrees).
> 
>    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>    Reviewed-by: Tom Musta <tommusta@gmail.com>
>    Tested-by: Tom Musta <tommusta@gmail.com>
>    Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> that conflicts (semantically) with this.

Phew, I can also take that patch off my queue again and apply Paolo's v3 in one go if that makes life easier for everyone again ;).


Alex

Re: [Qemu-devel] [PATCH 2/7] target-ppc: Introduce gen_set_cr1_from_fpscr

[Tom Musta]

On 11/4/2014 9:58 AM, Paolo Bonzini wrote:
> What tree are these patches based on?  Alex's tree already has a
> 
> commit 15a6b218c221a34b12e81790f427efec3108dce9
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Thu Aug 28 19:15:07 2014 +0200
> 
>     ppc: rename gen_set_cr6_from_fpscr
> 
>     It sets CR1, not CR6 (and the spec agrees).
> 
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Reviewed-by: Tom Musta <tommusta@gmail.com>
>     Tested-by: Tom Musta <tommusta@gmail.com>
>     Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> that conflicts (semantically) with this.
> 
> Paolo

Ahhh .. I had forgotten about that one.  My patches are based on master.

I will rebase on ppc-next and submit V2.

> 
> On 03/11/2014 21:01, Tom Musta wrote:
>> The Power ISA supports a mode in many floating point instructions whereby
>> the Condition Register field 1 (CR[1]) receives a copy of the Floating
>> Point Status (FPSCR) bits 32:35, also known as FX, FEX VX and OX.
>>
>> The existing QEMU code is mostly wrong -- CR[1] is set to the Floating
>> Point Condition Code (FPSCR[FPCC]).  Furthermore, this code is buried
>> inside the code that generates the FPSCR[FPRF] code, which is awkward.
>>
>> Introduce a new generator utility that correctly sets CR[1] from the
>> FPSCR bits.  Subsequent patches will correct various segments of
>> the defective code and will clean up the gen_compute_fprf()
>> utility.
>>
>> Signed-off-by: Tom Musta <tommusta@gmail.com>
>> ---
>>  target-ppc/translate.c |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> index d03daea..7775bf4 100644
>> --- a/target-ppc/translate.c
>> +++ b/target-ppc/translate.c
>> @@ -249,6 +249,14 @@ static inline void gen_reset_fpstatus(void)
>>      gen_helper_reset_fpstatus(cpu_env);
>>  }
>>  
>> +static inline void gen_set_cr1_from_fpscr(void)
>> +{
>> +    TCGv_i32 t0 = tcg_temp_new_i32();
>> +    tcg_gen_trunc_tl_i32(t0, cpu_fpscr);
>> +    tcg_gen_shri_i32(cpu_crf[1], t0, 28);
>> +    tcg_temp_free_i32(t0);
>> +}
>> +
>>  static inline void gen_compute_fprf(TCGv_i64 arg, int set_fprf, int set_rc)
>>  {
>>      TCGv_i32 t0 = tcg_temp_new_i32();
>>

Re: [Qemu-devel] [PATCH 2/7] target-ppc: Introduce gen_set_cr1_from_fpscr

[Paolo Bonzini]

On 04/11/2014 16:58, Paolo Bonzini wrote:
> What tree are these patches based on?  Alex's tree already has a
> 
> commit 15a6b218c221a34b12e81790f427efec3108dce9
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Thu Aug 28 19:15:07 2014 +0200
> 
>     ppc: rename gen_set_cr6_from_fpscr
> 
>     It sets CR1, not CR6 (and the spec agrees).
> 
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Reviewed-by: Tom Musta <tommusta@gmail.com>
>     Tested-by: Tom Musta <tommusta@gmail.com>
>     Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> that conflicts (semantically) with this.

Please take a look at branch tcg-ppc on
git://github.com/bonzini/qemu.git.  Hardly tested for now, will look
more at it tomorrow.

Paolo

Re: [Qemu-devel] [PATCH 2/7] target-ppc: Introduce gen_set_cr1_from_fpscr

[Paolo Bonzini]

On 04/11/2014 17:16, Alexander Graf wrote:
>> Am 04.11.2014 um 16:58 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>>
>> What tree are these patches based on?  Alex's tree already has a
>>
>> commit 15a6b218c221a34b12e81790f427efec3108dce9
>> Author: Paolo Bonzini <pbonzini@redhat.com>
>> Date:   Thu Aug 28 19:15:07 2014 +0200
>>
>>    ppc: rename gen_set_cr6_from_fpscr
>>
>>    It sets CR1, not CR6 (and the spec agrees).
>>
>>    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>    Reviewed-by: Tom Musta <tommusta@gmail.com>
>>    Tested-by: Tom Musta <tommusta@gmail.com>
>>    Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> that conflicts (semantically) with this.
> 
> Phew, I can also take that patch off my queue again and apply Paolo's v3 in one go if that makes life easier for everyone again ;).

It shouldn't be hard to keep that patch.

Paolo