Re: [Qemu-devel] [PATCH 13/14] ppc: inline ppc_set_crf when clearer

[Tom Musta <tommusta@gmail.com>]

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;
>