| Message ID | 1410793421-6453-14-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 9/15/2014 10:03 AM, Paolo Bonzini wrote: > Do not go through the loop when we're setting the four CR fields to > separate constants or conditions. This is clearer than putting together > 4-bit value and passing it. I guess "clearer" is in the eye of the beholder .... :) In general, replacing a single line of code with four is not a simplification (IMO). That said, I was not able to spot or identify by testing any functional problems with this patch. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > v1->v2: due to previous changes, ppc_get_crf never needs this > treatment, so I adjusted the subject > > linux-user/main.c | 5 ++++- > target-ppc/fpu_helper.c | 12 ++++++++++-- > target-ppc/int_helper.c | 27 +++++++++++++++++++++------ > 3 files changed, 35 insertions(+), 9 deletions(-) > > diff --git a/linux-user/main.c b/linux-user/main.c > index b403f24..5a0b31f 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -1550,7 +1550,10 @@ static int do_store_exclusive(CPUPPCState *env) > } > } > } > - ppc_set_crf(env, 0, (stored << 1) | xer_so); > + env->cr[CRF_LT] = 0; > + env->cr[CRF_GT] = 0; > + env->cr[CRF_EQ] = stored; > + env->cr[CRF_SO] = xer_so; > env->reserve_addr = (target_ulong)-1; > } > if (!segv) { > diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c > index 7894dc5..c86320f 100644 > --- a/target-ppc/fpu_helper.c > +++ b/target-ppc/fpu_helper.c > @@ -1099,7 +1099,11 @@ void helper_fcmpu(CPUPPCState *env, uint64_t arg1, uint64_t arg2, > > env->fpscr &= ~(0x0F << FPSCR_FPRF); > env->fpscr |= (0x08 << FPSCR_FPRF) >> fpcc; > - ppc_set_crf(env, crfD, 0x08 >> fpcc); > + > + env->cr[crfD * 4 + CRF_LT] = (fpcc == CRF_LT); > + env->cr[crfD * 4 + CRF_GT] = (fpcc == CRF_GT); > + env->cr[crfD * 4 + CRF_EQ] = (fpcc == CRF_EQ); > + env->cr[crfD * 4 + CRF_SO] = (fpcc == CRF_SO); > > if (unlikely(fpcc == CRF_SO > && (float64_is_signaling_nan(farg1.d) || > @@ -1131,7 +1135,11 @@ void helper_fcmpo(CPUPPCState *env, uint64_t arg1, uint64_t arg2, > > env->fpscr &= ~(0x0F << FPSCR_FPRF); > env->fpscr |= (0x08 << FPSCR_FPRF) >> fpcc; > - ppc_set_crf(env, crfD, 0x08 >> fpcc); > + > + env->cr[crfD * 4 + CRF_LT] = (fpcc == CRF_LT); > + env->cr[crfD * 4 + CRF_GT] = (fpcc == CRF_GT); > + env->cr[crfD * 4 + CRF_EQ] = (fpcc == CRF_EQ); > + env->cr[crfD * 4 + CRF_SO] = (fpcc == CRF_SO); > > if (unlikely(fpcc == CRF_SO)) { > if (float64_is_signaling_nan(farg1.d) || > diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c > index 96f2e7d..be52437 100644 > --- a/target-ppc/int_helper.c > +++ b/target-ppc/int_helper.c > @@ -657,7 +657,10 @@ VCF(sx, int32_to_float32, s32) > none |= result; \ > } \ > if (record) { \ > - ppc_set_crf(env, 6, ((all != 0) << 3) | ((none == 0) << 1)); \ > + env->cr[24 + CRF_LT] = (all != 0); \ > + env->cr[24 + CRF_GT] = 0; \ > + env->cr[24 + CRF_EQ] = (none == 0); \ > + env->cr[24 + CRF_SO] = 0; \ > } \ > } > #define VCMP(suffix, compare, element) \ > @@ -703,7 +706,10 @@ VCMP(gtsd, >, s64) > none |= result; \ > } \ > if (record) { \ > - ppc_set_crf(env, 6, ((all != 0) << 3) | ((none == 0) << 1)); \ > + env->cr[24 + CRF_LT] = (all != 0); \ > + env->cr[24 + CRF_GT] = 0; \ > + env->cr[24 + CRF_EQ] = (none == 0); \ > + env->cr[24 + CRF_SO] = 0; \ > } \ > } > #define VCMPFP(suffix, compare, order) \ > @@ -737,7 +743,10 @@ static inline void vcmpbfp_internal(CPUPPCState *env, ppc_avr_t *r, > } > } > if (record) { > - ppc_set_crf(env, 6, (all_in == 0) << 1); > + env->cr[24 + CRF_LT] = 0; > + env->cr[24 + CRF_GT] = 0; > + env->cr[24 + CRF_EQ] = (all_in == 0); > + env->cr[24 + CRF_SO] = 0; > } > } > > @@ -2558,7 +2567,9 @@ target_ulong helper_dlmzb(CPUPPCState *env, target_ulong high, > for (mask = 0xFF000000; mask != 0; mask = mask >> 8) { > if ((high & mask) == 0) { > if (update_Rc) { > - ppc_set_crf(env, 0, 0x4); > + env->cr[CRF_LT] = 0; > + env->cr[CRF_GT] = 1; > + env->cr[CRF_EQ] = 0; > } > goto done; > } > @@ -2567,7 +2578,9 @@ target_ulong helper_dlmzb(CPUPPCState *env, target_ulong high, > for (mask = 0xFF000000; mask != 0; mask = mask >> 8) { > if ((low & mask) == 0) { > if (update_Rc) { > - ppc_set_crf(env, 0, 0x8); > + env->cr[CRF_LT] = 1; > + env->cr[CRF_GT] = 0; > + env->cr[CRF_EQ] = 0; > } > goto done; > } > @@ -2575,7 +2588,9 @@ target_ulong helper_dlmzb(CPUPPCState *env, target_ulong high, > } > i = 8; > if (update_Rc) { > - ppc_set_crf(env, 0, 0x2); > + env->cr[CRF_LT] = 0; > + env->cr[CRF_GT] = 0; > + env->cr[CRF_EQ] = 1; > } > done: > env->xer = (env->xer & ~0x7F) | i; >
Il 18/09/2014 22:33, Tom Musta ha scritto: >> > Do not go through the loop when we're setting the four CR fields to >> > separate constants or conditions. This is clearer than putting together >> > 4-bit value and passing it. > I guess "clearer" is in the eye of the beholder .... :) > > In general, replacing a single line of code with four is not a simplification (IMO). > > That said, I was not able to spot or identify by testing any functional problems with this patch. > True. It is better to say that it avoids messing with bit endianness. Paolo
diff --git a/linux-user/main.c b/linux-user/main.c index b403f24..5a0b31f 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -1550,7 +1550,10 @@ static int do_store_exclusive(CPUPPCState *env) } } } - ppc_set_crf(env, 0, (stored << 1) | xer_so); + env->cr[CRF_LT] = 0; + env->cr[CRF_GT] = 0; + env->cr[CRF_EQ] = stored; + env->cr[CRF_SO] = xer_so; env->reserve_addr = (target_ulong)-1; } if (!segv) { diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c index 7894dc5..c86320f 100644 --- a/target-ppc/fpu_helper.c +++ b/target-ppc/fpu_helper.c @@ -1099,7 +1099,11 @@ void helper_fcmpu(CPUPPCState *env, uint64_t arg1, uint64_t arg2, env->fpscr &= ~(0x0F << FPSCR_FPRF); env->fpscr |= (0x08 << FPSCR_FPRF) >> fpcc; - ppc_set_crf(env, crfD, 0x08 >> fpcc); + + env->cr[crfD * 4 + CRF_LT] = (fpcc == CRF_LT); + env->cr[crfD * 4 + CRF_GT] = (fpcc == CRF_GT); + env->cr[crfD * 4 + CRF_EQ] = (fpcc == CRF_EQ); + env->cr[crfD * 4 + CRF_SO] = (fpcc == CRF_SO); if (unlikely(fpcc == CRF_SO && (float64_is_signaling_nan(farg1.d) || @@ -1131,7 +1135,11 @@ void helper_fcmpo(CPUPPCState *env, uint64_t arg1, uint64_t arg2, env->fpscr &= ~(0x0F << FPSCR_FPRF); env->fpscr |= (0x08 << FPSCR_FPRF) >> fpcc; - ppc_set_crf(env, crfD, 0x08 >> fpcc); + + env->cr[crfD * 4 + CRF_LT] = (fpcc == CRF_LT); + env->cr[crfD * 4 + CRF_GT] = (fpcc == CRF_GT); + env->cr[crfD * 4 + CRF_EQ] = (fpcc == CRF_EQ); + env->cr[crfD * 4 + CRF_SO] = (fpcc == CRF_SO); if (unlikely(fpcc == CRF_SO)) { if (float64_is_signaling_nan(farg1.d) || diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c index 96f2e7d..be52437 100644 --- a/target-ppc/int_helper.c +++ b/target-ppc/int_helper.c @@ -657,7 +657,10 @@ VCF(sx, int32_to_float32, s32) none |= result; \ } \ if (record) { \ - ppc_set_crf(env, 6, ((all != 0) << 3) | ((none == 0) << 1)); \ + env->cr[24 + CRF_LT] = (all != 0); \ + env->cr[24 + CRF_GT] = 0; \ + env->cr[24 + CRF_EQ] = (none == 0); \ + env->cr[24 + CRF_SO] = 0; \ } \ } #define VCMP(suffix, compare, element) \ @@ -703,7 +706,10 @@ VCMP(gtsd, >, s64) none |= result; \ } \ if (record) { \ - ppc_set_crf(env, 6, ((all != 0) << 3) | ((none == 0) << 1)); \ + env->cr[24 + CRF_LT] = (all != 0); \ + env->cr[24 + CRF_GT] = 0; \ + env->cr[24 + CRF_EQ] = (none == 0); \ + env->cr[24 + CRF_SO] = 0; \ } \ } #define VCMPFP(suffix, compare, order) \ @@ -737,7 +743,10 @@ static inline void vcmpbfp_internal(CPUPPCState *env, ppc_avr_t *r, } } if (record) { - ppc_set_crf(env, 6, (all_in == 0) << 1); + env->cr[24 + CRF_LT] = 0; + env->cr[24 + CRF_GT] = 0; + env->cr[24 + CRF_EQ] = (all_in == 0); + env->cr[24 + CRF_SO] = 0; } } @@ -2558,7 +2567,9 @@ target_ulong helper_dlmzb(CPUPPCState *env, target_ulong high, for (mask = 0xFF000000; mask != 0; mask = mask >> 8) { if ((high & mask) == 0) { if (update_Rc) { - ppc_set_crf(env, 0, 0x4); + env->cr[CRF_LT] = 0; + env->cr[CRF_GT] = 1; + env->cr[CRF_EQ] = 0; } goto done; } @@ -2567,7 +2578,9 @@ target_ulong helper_dlmzb(CPUPPCState *env, target_ulong high, for (mask = 0xFF000000; mask != 0; mask = mask >> 8) { if ((low & mask) == 0) { if (update_Rc) { - ppc_set_crf(env, 0, 0x8); + env->cr[CRF_LT] = 1; + env->cr[CRF_GT] = 0; + env->cr[CRF_EQ] = 0; } goto done; } @@ -2575,7 +2588,9 @@ target_ulong helper_dlmzb(CPUPPCState *env, target_ulong high, } i = 8; if (update_Rc) { - ppc_set_crf(env, 0, 0x2); + env->cr[CRF_LT] = 0; + env->cr[CRF_GT] = 0; + env->cr[CRF_EQ] = 1; } done: env->xer = (env->xer & ~0x7F) | i;
Do not go through the loop when we're setting the four CR fields to separate constants or conditions. This is clearer than putting together 4-bit value and passing it. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- v1->v2: due to previous changes, ppc_get_crf never needs this treatment, so I adjusted the subject linux-user/main.c | 5 ++++- target-ppc/fpu_helper.c | 12 ++++++++++-- target-ppc/int_helper.c | 27 +++++++++++++++++++++------ 3 files changed, 35 insertions(+), 9 deletions(-)