* [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup
@ 2014-11-12 21:45 Tom Musta
2014-11-12 21:45 ` [Qemu-devel] [2.3 V2 PATCH 1/6] target-ppc: VXSQRT Should Not Be Set for NaNs Tom Musta
` (6 more replies)
0 siblings, 7 replies; 11+ messages in thread
From: Tom Musta @ 2014-11-12 21:45 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Tom Musta, agraf
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 NaN whose sign bit is a 1.
Patches 2-6 correct a rather pervasive problem with modeling of the CR[1]
field (i.e. the "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?
V2 Reworked patches to pick up the gen_set_cr1_from_fpscr() utility that
was recently added by Paolo Bonzini.
Tom Musta (6):
target-ppc: VXSQRT Should Not Be Set for NaNs
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 | 131 ++++++++++++++++++++++++++++-------------------
3 files changed, 122 insertions(+), 96 deletions(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [2.3 V2 PATCH 1/6] target-ppc: VXSQRT Should Not Be Set for NaNs
2014-11-12 21:45 [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Tom Musta
@ 2014-11-12 21:45 ` Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1 Tom Musta
` (5 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Tom Musta @ 2014-11-12 21:45 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Tom Musta, agraf
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 7f74466..81db60f 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)
@@ -920,14 +923,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;
@@ -974,17 +979,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;
}
@@ -2382,9 +2390,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1
2014-11-12 21:45 [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Tom Musta
2014-11-12 21:45 ` [Qemu-devel] [2.3 V2 PATCH 1/6] target-ppc: VXSQRT Should Not Be Set for NaNs Tom Musta
@ 2014-11-12 21:46 ` Tom Musta
2014-11-20 14:14 ` Alexander Graf
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 3/6] target-ppc: mffs. Should Set CR1 from FPSCR Bits Tom Musta
` (4 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Tom Musta @ 2014-11-12 21:46 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Tom Musta, agraf
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 gen_set_cr1_from_fpscr utility.
Signed-off-by: Tom Musta <tommusta@gmail.com>
---
target-ppc/translate.c | 50 ++++++++++++++++++++++++++++-------------------
1 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 910ce56..2d79e39 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -2077,6 +2077,21 @@ static void gen_srd(DisasContext *ctx)
}
#endif
+#if defined(TARGET_PPC64)
+static void gen_set_cr1_from_fpscr(DisasContext *ctx)
+{
+ TCGv_i32 tmp = tcg_temp_new_i32();
+ tcg_gen_trunc_tl_i32(tmp, cpu_fpscr);
+ tcg_gen_shri_i32(cpu_crf[1], tmp, 28);
+ tcg_temp_free_i32(tmp);
+}
+#else
+static void gen_set_cr1_from_fpscr(DisasContext *ctx)
+{
+ tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
+}
+#endif
+
/*** Floating-Point arithmetic ***/
#define _GEN_FLOAT_ACB(name, op, op1, op2, isfloat, set_fprf, type) \
static void gen_f##name(DisasContext *ctx) \
@@ -2370,7 +2385,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(ctx);
+ }
}
/* fmr - fmr. */
@@ -2382,7 +2399,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(ctx);
+ }
}
/* fnabs */
@@ -2395,7 +2414,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(ctx);
+ }
}
/* fneg */
@@ -2408,7 +2429,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(ctx);
+ }
}
/* fcpsgn: PowerPC 2.05 specification */
@@ -2421,7 +2444,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(ctx);
+ }
}
static void gen_fmrgew(DisasContext *ctx)
@@ -8205,21 +8230,6 @@ static inline TCGv_ptr gen_fprp_ptr(int reg)
return r;
}
-#if defined(TARGET_PPC64)
-static void gen_set_cr1_from_fpscr(DisasContext *ctx)
-{
- TCGv_i32 tmp = tcg_temp_new_i32();
- tcg_gen_trunc_tl_i32(tmp, cpu_fpscr);
- tcg_gen_shri_i32(cpu_crf[1], tmp, 28);
- tcg_temp_free_i32(tmp);
-}
-#else
-static void gen_set_cr1_from_fpscr(DisasContext *ctx)
-{
- tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
-}
-#endif
-
#define GEN_DFP_T_A_B_Rc(name) \
static void gen_##name(DisasContext *ctx) \
{ \
--
1.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [2.3 V2 PATCH 3/6] target-ppc: mffs. Should Set CR1 from FPSCR Bits
2014-11-12 21:45 [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Tom Musta
2014-11-12 21:45 ` [Qemu-devel] [2.3 V2 PATCH 1/6] target-ppc: VXSQRT Should Not Be Set for NaNs Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1 Tom Musta
@ 2014-11-12 21:46 ` Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 4/6] target-ppc: Fully Migrate to gen_set_cr1_from_fpscr Tom Musta
` (3 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Tom Musta @ 2014-11-12 21:46 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Tom Musta, agraf
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 2d79e39..f3c57b8 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -2504,7 +2504,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(ctx);
+ }
}
/* mtfsb0 */
--
1.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [2.3 V2 PATCH 4/6] target-ppc: Fully Migrate to gen_set_cr1_from_fpscr
2014-11-12 21:45 [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Tom Musta
` (2 preceding siblings ...)
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 3/6] target-ppc: mffs. Should Set CR1 from FPSCR Bits Tom Musta
@ 2014-11-12 21:46 ` Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 5/6] target-ppc: Eliminate set_fprf Argument From gen_compute_fprf Tom Musta
` (2 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Tom Musta @ 2014-11-12 21:46 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Tom Musta, agraf
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 f3c57b8..d4faf20 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -250,7 +250,7 @@ static inline void gen_reset_fpstatus(void)
gen_helper_reset_fpstatus(cpu_env);
}
-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();
@@ -258,15 +258,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);
@@ -2110,8 +2102,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(ctx); \
+ } \
}
#define GEN_FLOAT_ACB(name, op2, set_fprf, type) \
@@ -2135,8 +2129,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(ctx); \
+ } \
}
#define GEN_FLOAT_AB(name, op2, inval, set_fprf, type) \
_GEN_FLOAT_AB(name, name, 0x3F, op2, inval, 0, set_fprf, type); \
@@ -2159,8 +2155,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(ctx); \
+ } \
}
#define GEN_FLOAT_AC(name, op2, inval, set_fprf, type) \
_GEN_FLOAT_AC(name, name, 0x3F, op2, inval, 0, set_fprf, type); \
@@ -2178,8 +2176,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(ctx); \
+ } \
}
#define GEN_FLOAT_BS(name, op1, op2, set_fprf, type) \
@@ -2194,8 +2194,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(ctx); \
+ } \
}
/* fadd - fadds */
@@ -2228,7 +2230,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(ctx);
+ }
}
/* fsel */
@@ -2249,7 +2254,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(ctx);
+ }
}
static void gen_fsqrts(DisasContext *ctx)
@@ -2265,7 +2273,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(ctx);
+ }
}
/*** Floating-Point multiply-and-add ***/
--
1.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [2.3 V2 PATCH 5/6] target-ppc: Eliminate set_fprf Argument From gen_compute_fprf
2014-11-12 21:45 [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Tom Musta
` (3 preceding siblings ...)
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 4/6] target-ppc: Fully Migrate to gen_set_cr1_from_fpscr Tom Musta
@ 2014-11-12 21:46 ` Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 6/6] target-ppc: Eliminate set_fprf Argument From helper_compute_fprf Tom Musta
2014-11-20 14:51 ` [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Alexander Graf
6 siblings, 0 replies; 11+ messages in thread
From: Tom Musta @ 2014-11-12 21:46 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Tom Musta, agraf
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 d4faf20..9ac9f43 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -250,16 +250,14 @@ static inline void gen_reset_fpstatus(void)
gen_helper_reset_fpstatus(cpu_env);
}
-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);
}
@@ -2102,7 +2100,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(ctx); \
} \
@@ -2129,7 +2129,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(ctx); \
} \
@@ -2155,7 +2157,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(ctx); \
} \
@@ -2176,7 +2180,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(ctx); \
} \
@@ -2194,7 +2200,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(ctx); \
} \
@@ -2230,7 +2238,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(ctx);
}
@@ -2254,7 +2262,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(ctx);
}
@@ -2273,7 +2281,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(ctx);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [2.3 V2 PATCH 6/6] target-ppc: Eliminate set_fprf Argument From helper_compute_fprf
2014-11-12 21:45 [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Tom Musta
` (4 preceding siblings ...)
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 5/6] target-ppc: Eliminate set_fprf Argument From gen_compute_fprf Tom Musta
@ 2014-11-12 21:46 ` Tom Musta
2014-11-20 14:51 ` [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Alexander Graf
6 siblings, 0 replies; 11+ messages in thread
From: Tom Musta @ 2014-11-12 21:46 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Tom Musta, agraf
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 81db60f..6cceffc 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 */
@@ -1853,7 +1849,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); \
@@ -1908,7 +1904,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode) \
} \
\
if (sfprf) { \
- helper_compute_fprf(env, xt.fld, sfprf); \
+ helper_compute_fprf(env, xt.fld); \
} \
} \
\
@@ -1962,7 +1958,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode) \
} \
\
if (sfprf) { \
- helper_compute_fprf(env, xt.fld, sfprf); \
+ helper_compute_fprf(env, xt.fld); \
} \
} \
\
@@ -2003,7 +1999,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode) \
} \
\
if (sfprf) { \
- helper_compute_fprf(env, xt.fld, sfprf); \
+ helper_compute_fprf(env, xt.fld); \
} \
} \
\
@@ -2052,7 +2048,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode) \
} \
\
if (sfprf) { \
- helper_compute_fprf(env, xt.fld, sfprf); \
+ helper_compute_fprf(env, xt.fld); \
} \
} \
\
@@ -2102,7 +2098,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode) \
} \
\
if (sfprf) { \
- helper_compute_fprf(env, xt.fld, sfprf); \
+ helper_compute_fprf(env, xt.fld); \
} \
} \
\
@@ -2302,7 +2298,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); \
@@ -2509,7 +2505,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)); \
} \
} \
\
@@ -2619,7 +2615,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); \
} \
} \
\
@@ -2674,7 +2670,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); \
} \
} \
\
@@ -2714,7 +2710,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 210fd97..2841f61 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 9ac9f43..0f8897f 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -252,14 +252,8 @@ static inline void gen_reset_fpstatus(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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1 Tom Musta
@ 2014-11-20 14:14 ` Alexander Graf
2014-11-20 14:32 ` Tom Musta
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Graf @ 2014-11-20 14:14 UTC (permalink / raw)
To: Tom Musta, qemu-devel, qemu-ppc
On 12.11.14 22:46, Tom Musta wrote:
> 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 gen_set_cr1_from_fpscr utility.
>
> Signed-off-by: Tom Musta <tommusta@gmail.com>
> ---
> target-ppc/translate.c | 50 ++++++++++++++++++++++++++++-------------------
> 1 files changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 910ce56..2d79e39 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -2077,6 +2077,21 @@ static void gen_srd(DisasContext *ctx)
> }
> #endif
>
> +#if defined(TARGET_PPC64)
> +static void gen_set_cr1_from_fpscr(DisasContext *ctx)
> +{
> + TCGv_i32 tmp = tcg_temp_new_i32();
> + tcg_gen_trunc_tl_i32(tmp, cpu_fpscr);
> + tcg_gen_shri_i32(cpu_crf[1], tmp, 28);
> + tcg_temp_free_i32(tmp);
> +}
> +#else
> +static void gen_set_cr1_from_fpscr(DisasContext *ctx)
> +{
> + tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
> +}
> +#endif
> +
> /*** Floating-Point arithmetic ***/
> #define _GEN_FLOAT_ACB(name, op, op1, op2, isfloat, set_fprf, type) \
> static void gen_f##name(DisasContext *ctx) \
> @@ -2370,7 +2385,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(ctx);
> + }
I don't quite understand this. We set cr1 based on fpscr, but we don't
recalculate the respective fpscr bits?
Wouldn't we get outdated comparison data?
Alex
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1
2014-11-20 14:14 ` Alexander Graf
@ 2014-11-20 14:32 ` Tom Musta
2014-11-20 14:49 ` Alexander Graf
0 siblings, 1 reply; 11+ messages in thread
From: Tom Musta @ 2014-11-20 14:32 UTC (permalink / raw)
To: Alexander Graf, qemu-devel, qemu-ppc
On 11/20/2014 8:14 AM, Alexander Graf wrote:
>
>
> On 12.11.14 22:46, Tom Musta wrote:
>> 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 gen_set_cr1_from_fpscr utility.
>>
>> Signed-off-by: Tom Musta <tommusta@gmail.com>
>> ---
>> target-ppc/translate.c | 50 ++++++++++++++++++++++++++++-------------------
>> 1 files changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> index 910ce56..2d79e39 100644
>> --- a/target-ppc/translate.c
>> +++ b/target-ppc/translate.c
>> @@ -2077,6 +2077,21 @@ static void gen_srd(DisasContext *ctx)
>> }
>> #endif
>>
>> +#if defined(TARGET_PPC64)
>> +static void gen_set_cr1_from_fpscr(DisasContext *ctx)
>> +{
>> + TCGv_i32 tmp = tcg_temp_new_i32();
>> + tcg_gen_trunc_tl_i32(tmp, cpu_fpscr);
>> + tcg_gen_shri_i32(cpu_crf[1], tmp, 28);
>> + tcg_temp_free_i32(tmp);
>> +}
>> +#else
>> +static void gen_set_cr1_from_fpscr(DisasContext *ctx)
>> +{
>> + tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
>> +}
>> +#endif
>> +
>> /*** Floating-Point arithmetic ***/
>> #define _GEN_FLOAT_ACB(name, op, op1, op2, isfloat, set_fprf, type) \
>> static void gen_f##name(DisasContext *ctx) \
>> @@ -2370,7 +2385,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(ctx);
>> + }
>
> I don't quite understand this. We set cr1 based on fpscr, but we don't
> recalculate the respective fpscr bits?
>
> Wouldn't we get outdated comparison data?
>
>
> Alex
>
Nope.
The floating point move instructions don't actually even alter the FPSCR. From the ISA (see the last sentence):
4.6.5 Floating-Point Move Instructions
These instructions copy data from one floating-point
register to another, altering the sign bit (bit 0) as
described below for fneg, fabs, fnabs, and fcpsgn.
These instructions treat NaNs just like any other kind of
value (e.g., the sign bit of a NaN may be altered by
fneg, fabs, fnabs, and fcpsgn). These instructions do
not alter the FPSCR.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1
2014-11-20 14:32 ` Tom Musta
@ 2014-11-20 14:49 ` Alexander Graf
0 siblings, 0 replies; 11+ messages in thread
From: Alexander Graf @ 2014-11-20 14:49 UTC (permalink / raw)
To: Tom Musta, qemu-devel, qemu-ppc
On 20.11.14 15:32, Tom Musta wrote:
> On 11/20/2014 8:14 AM, Alexander Graf wrote:
>>
>>
>> On 12.11.14 22:46, Tom Musta wrote:
>>> 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 gen_set_cr1_from_fpscr utility.
>>>
>>> Signed-off-by: Tom Musta <tommusta@gmail.com>
>>> ---
>>> target-ppc/translate.c | 50 ++++++++++++++++++++++++++++-------------------
>>> 1 files changed, 30 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>> index 910ce56..2d79e39 100644
>>> --- a/target-ppc/translate.c
>>> +++ b/target-ppc/translate.c
>>> @@ -2077,6 +2077,21 @@ static void gen_srd(DisasContext *ctx)
>>> }
>>> #endif
>>>
>>> +#if defined(TARGET_PPC64)
>>> +static void gen_set_cr1_from_fpscr(DisasContext *ctx)
>>> +{
>>> + TCGv_i32 tmp = tcg_temp_new_i32();
>>> + tcg_gen_trunc_tl_i32(tmp, cpu_fpscr);
>>> + tcg_gen_shri_i32(cpu_crf[1], tmp, 28);
>>> + tcg_temp_free_i32(tmp);
>>> +}
>>> +#else
>>> +static void gen_set_cr1_from_fpscr(DisasContext *ctx)
>>> +{
>>> + tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
>>> +}
>>> +#endif
>>> +
>>> /*** Floating-Point arithmetic ***/
>>> #define _GEN_FLOAT_ACB(name, op, op1, op2, isfloat, set_fprf, type) \
>>> static void gen_f##name(DisasContext *ctx) \
>>> @@ -2370,7 +2385,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(ctx);
>>> + }
>>
>> I don't quite understand this. We set cr1 based on fpscr, but we don't
>> recalculate the respective fpscr bits?
>>
>> Wouldn't we get outdated comparison data?
>>
>>
>> Alex
>>
>
> Nope.
>
> The floating point move instructions don't actually even alter the FPSCR. From the ISA (see the last sentence):
>
> 4.6.5 Floating-Point Move Instructions
> These instructions copy data from one floating-point
> register to another, altering the sign bit (bit 0) as
> described below for fneg, fabs, fnabs, and fcpsgn.
> These instructions treat NaNs just like any other kind of
> value (e.g., the sign bit of a NaN may be altered by
> fneg, fabs, fnabs, and fcpsgn). These instructions do
> not alter the FPSCR.
Thanks, applied with the following squashed in:
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 7aef089..35c3a16 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -2088,7 +2088,7 @@ static void gen_set_cr1_from_fpscr(DisasContext *ctx)
#else
static void gen_set_cr1_from_fpscr(DisasContext *ctx)
{
- tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
+ tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
}
#endif
Alex
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup
2014-11-12 21:45 [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Tom Musta
` (5 preceding siblings ...)
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 6/6] target-ppc: Eliminate set_fprf Argument From helper_compute_fprf Tom Musta
@ 2014-11-20 14:51 ` Alexander Graf
6 siblings, 0 replies; 11+ messages in thread
From: Alexander Graf @ 2014-11-20 14:51 UTC (permalink / raw)
To: Tom Musta, qemu-devel, qemu-ppc
On 12.11.14 22:45, Tom Musta wrote:
> 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 NaN whose sign bit is a 1.
>
> Patches 2-6 correct a rather pervasive problem with modeling of the CR[1]
> field (i.e. the "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?
>
> V2 Reworked patches to pick up the gen_set_cr1_from_fpscr() utility that
> was recently added by Paolo Bonzini.
Thanks, applied all to ppc-next-2.3.
Alex
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-11-20 14:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-12 21:45 [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Tom Musta
2014-11-12 21:45 ` [Qemu-devel] [2.3 V2 PATCH 1/6] target-ppc: VXSQRT Should Not Be Set for NaNs Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1 Tom Musta
2014-11-20 14:14 ` Alexander Graf
2014-11-20 14:32 ` Tom Musta
2014-11-20 14:49 ` Alexander Graf
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 3/6] target-ppc: mffs. Should Set CR1 from FPSCR Bits Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 4/6] target-ppc: Fully Migrate to gen_set_cr1_from_fpscr Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 5/6] target-ppc: Eliminate set_fprf Argument From gen_compute_fprf Tom Musta
2014-11-12 21:46 ` [Qemu-devel] [2.3 V2 PATCH 6/6] target-ppc: Eliminate set_fprf Argument From helper_compute_fprf Tom Musta
2014-11-20 14:51 ` [Qemu-devel] [2.3 V2 PATCH 0/6] target-ppc: Assorted Floating Point Bugs and Cleanup Alexander Graf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).