target-ppc: Assorted Floating Point Bugs and Cleanup
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 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(-)
PATCH 1/7 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)
@@ -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)
Signature · 1 line
1.7.1
PATCH 2/7 target-ppc: Introduce gen_set_cr1_from_fpscr
In reply to Tom MustaThe 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.
--- target-ppc/translate.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
target-ppc/translate.c +8 -0
@@ -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 ();
Signature · 1 line
1.7.1
Re: [Qemu-devel] [PATCH 2/7] target-ppc: Introduce gen_set_cr1_from_fpscr
What tree are these patches based on? Alex's tree already has a
commit 15a6b218c221a34b12e81790f427efec3108dce9
Author: Paolo Bonzini <[email protected]> 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).
that conflicts (semantically) with this.
Paolo
Quoted 38 lines · 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 <[email protected]>--- target-ppc/translate.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)target-ppc/translate.c +8
-0@@ -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
Quoted 13 lines
Am 04.11.2014 um 16:58 schrieb Paolo Bonzini <[email protected]>:
What tree are these patches based on? Alex's tree already has a
commit
15a6b218c221a34b12e81790f427efec3108dce9Author: Paolo Bonzini <[email protected]> Date: Thu Aug 28 19:15:07 2014 +0200ppc: rename
gen_set_cr6_from_fpscr
Quoted 8 lines
It sets CR1, not CR6 (and the spec agrees).
Signed-off-by Paolo Bonzini <[email protected]>Reviewed-by Tom Musta <[email protected]>Tested-by Tom Musta <[email protected]>Signed-off-by Alexander Graf <[email protected]>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
Quoted 12 lines · Alexander Graf
Quoted 12 lines
Am 04.11.2014 um 16:58 schrieb Paolo Bonzini <[email protected]>:
What tree are these patches based on? Alex's tree already has a
commit
15a6b218c221a34b12e81790f427efec3108dce9Author: Paolo Bonzini <[email protected]> Date: Thu Aug 28 19:15:07 2014 +0200ppc: rename
gen_set_cr6_from_fpscrIt sets CR1, not CR6 (and the spec agrees).
Quoted 8 lines
Quoted 6 lines
Signed-off-by Paolo Bonzini <[email protected]>Reviewed-by Tom Musta <[email protected]>Tested-by Tom Musta <[email protected]>Signed-off-by Alexander Graf <[email protected]>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
Re: [Qemu-devel] [PATCH 2/7] target-ppc: Introduce gen_set_cr1_from_fpscr
In reply to Paolo BonziniQuoted 10 lines · Paolo Bonzini
What tree are these patches based on? Alex's tree already has a
commit
15a6b218c221a34b12e81790f427efec3108dce9Author: Paolo Bonzini <[email protected]> Date: Thu Aug 28 19:15:07 2014 +0200ppc: rename
gen_set_cr6_from_fpscrIt sets CR1, not CR6 (and the spec agrees).
Quoted 8 lines
Signed-off-by Paolo Bonzini <[email protected]>Reviewed-by Tom Musta <[email protected]>Tested-by Tom Musta <[email protected]>Signed-off-by Alexander Graf <[email protected]>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.
Quoted 40 lines · Tom Musta
Quoted 38 lines
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 <[email protected]>--- target-ppc/translate.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)target-ppc/translate.c +8
-0@@ -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
In reply to Paolo BonziniQuoted 8 lines · Paolo Bonzini
What tree are these patches based on? Alex's tree already has a
commit
15a6b218c221a34b12e81790f427efec3108dce9Author: Paolo Bonzini <[email protected]> Date: Thu Aug 28 19:15:07 2014 +0200ppc: rename
gen_set_cr6_from_fpscr
Quoted 8 lines
It sets CR1, not CR6 (and the spec agrees).
Signed-off-by Paolo Bonzini <[email protected]>Reviewed-by Tom Musta <[email protected]>Tested-by Tom Musta <[email protected]>Signed-off-by Alexander Graf <[email protected]>that conflicts (semantically) with this.
Please take a look at branch tcg-ppc ongit://github.com/bonzini/qemu.git. Hardly tested for now, will look more at it tomorrow.
Paolo
PATCH 3/7 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 newly added gen_set_cr1_from_fpscr utility.
--- target-ppc/translate.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)
target-ppc/translate.c +15 -5
@@ -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)
Signature · 1 line
1.7.1
PATCH 4/7 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
@@ -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 */
Signature · 1 line
1.7.1
PATCH 5/7 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
@@ -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 ***/
Signature · 1 line
1.7.1
PATCH 6/7 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
@@ -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 (); }
Signature · 1 line
1.7.1
PATCH 7/7 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 */
@@ -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; }
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
@@ -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)
Signature · 1 line
1.7.1
Quoted 3 lines · Tom Musta
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