target-ppc: Assorted Floating Point Bugs and Cleanup
v2 cover target-ppc: Assorted Floating Point Bugs and Cleanup
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(-)
PATCH v2 1/6 target-ppc: VXSQRT Should Not Be Set for NaNs
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.
--- target-ppc/fpu_helper.c | 29 +++++++++++++++++------------ 1 files changed, 17 insertions(+), 12 deletions(-)
target-ppc/fpu_helper.c +17 -12
@@ -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)
Signature · 1 line
1.7.1
PATCH v2 2/6 target-ppc: Fix Floating Point Move Instructions That Set CR1
In reply to Tom MustaThe 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.
--- target-ppc/translate.c | 50 ++++++++++++++++++++++++++++------------------- 1 files changed, 30 insertions(+), 20 deletions(-)
target-ppc/translate.c +30 -20
@@ -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) \ { \
Signature · 1 line
1.7.1
Re: [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1
Quoted 39 lines · 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
gen_set_cr1_from_fpscrutility.Signed-off-by Tom Musta <[email protected]>--- target-ppc/translate.c | 50 ++++++++++++++++++++++++++++------------------- 1 files changed, 30 insertions(+), 20 deletions(-)target-ppc/translate.c +15
-0@@ -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) \
target-ppc/translate.c static void gen_fabs(DisasContext *ctx) · line 2390
Quoted 7 lines
target-ppc/translate.c +3
-1@@ -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
Re: [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1
Quoted 53 lines · Alexander Graf
Quoted 39 lines · 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
gen_set_cr1_from_fpscrutility.Signed-off-by Tom Musta <[email protected]>--- target-ppc/translate.c | 50 ++++++++++++++++++++++++++++------------------- 1 files changed, 30 insertions(+), 20 deletions(-)target-ppc/translate.c +15
-0@@ -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) \target-ppc/translate.c static void gen_fabs(DisasContext *ctx) · line 2390
Quoted 7 lines
target-ppc/translate.c +3
-1@@ -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?
Quoted 5 lines
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.
Alexander Graf
AppliedRe: [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1
Quoted 64 lines · Tom Musta
Quoted 53 lines · Alexander Graf
Quoted 39 lines · 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
gen_set_cr1_from_fpscrutility.Signed-off-by Tom Musta <[email protected]>--- target-ppc/translate.c | 50 ++++++++++++++++++++++++++++------------------- 1 files changed, 30 insertions(+), 20 deletions(-)target-ppc/translate.c +15
-0@@ -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) \target-ppc/translate.c static void gen_fabs(DisasContext *ctx) · line 2390
Quoted 7 lines
target-ppc/translate.c +3
-1@@ -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?
Quoted 5 lines
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):
Quoted 8 lines
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:
target-ppc/translate.c +1 -1
@@ -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
PATCH v2 3/6 target-ppc: mffs. Should Set CR1 from FPSCR Bits
In reply to Tom MustaUpdate the Move From FPSCR (mffs.) instruction to correctly set CR[1] from FPSCR[FX,FEX,VX,OX].
--- target-ppc/translate.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
target-ppc/translate.c +3 -1
@@ -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 */
Signature · 1 line
1.7.1
PATCH v2 4/6 target-ppc: Fully Migrate to gen_set_cr1_from_fpscr
In reply to Tom MustaEliminate 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.
--- target-ppc/translate.c | 55 ++++++++++++++++++++++++++++------------------- 1 files changed, 33 insertions(+), 22 deletions(-)
target-ppc/translate.c +33 -22
@@ -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 ***/
Signature · 1 line
1.7.1
PATCH v2 5/6 target-ppc: Eliminate set_fprf Argument From gen_compute_fprf
In reply to Tom MustaThe 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.
--- target-ppc/translate.c | 38 +++++++++++++++++++++++--------------- 1 files changed, 23 insertions(+), 15 deletions(-)
target-ppc/translate.c +23 -15
@@ -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); }
Signature · 1 line
1.7.1
PATCH v2 6/6 target-ppc: Eliminate set_fprf Argument From helper_compute_fprf
In reply to Tom MustaThe 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.
--- target-ppc/fpu_helper.c | 56 +++++++++++++++++++++------------------------- target-ppc/helper.h | 2 +- target-ppc/translate.c | 8 +------ 3 files changed, 28 insertions(+), 38 deletions(-)
target-ppc/fpu_helper.c +26 -30
@@ -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; }
target-ppc/helper.h +1 -1
@@ -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)
target-ppc/translate.c +1 -7
@@ -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)
Signature · 1 line
1.7.1
Alexander Graf
AppliedQuoted 12 lines · 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 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.
Quoted 6 lines
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