| Message ID | 1415828764-10582-3-git-send-email-tommusta@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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) \ { \
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(-)