Re: [Qemu-devel] [PATCH 05/17] ppc: use CRF_* in fpu_helper.c

[Tom Musta <tommusta@gmail.com>]

On 8/28/2014 12:15 PM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-ppc/fpu_helper.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> index da93d12..0fe006a 100644
> --- a/target-ppc/fpu_helper.c
> +++ b/target-ppc/fpu_helper.c
> @@ -1043,7 +1043,7 @@ uint32_t helper_ftdiv(uint64_t fra, uint64_t frb)
>          }
>      }
>  
> -    return 0x8 | (fg_flag ? 4 : 0) | (fe_flag ? 2 : 0);
> +    return (1 << CRF_LT) | (fg_flag << CRF_GT) | (fe_flag << CRF_EQ);
>  }
>  
>  uint32_t helper_ftsqrt(uint64_t frb)
> @@ -1074,7 +1074,7 @@ uint32_t helper_ftsqrt(uint64_t frb)
>          }
>      }
>  
> -    return 0x8 | (fg_flag ? 4 : 0) | (fe_flag ? 2 : 0);
> +    return (1 << CRF_LT) | (fg_flag << CRF_GT) | (fe_flag << CRF_EQ);
>  }
>  
>  void helper_fcmpu(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
> @@ -1088,19 +1088,19 @@ void helper_fcmpu(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
>  
>      if (unlikely(float64_is_any_nan(farg1.d) ||
>                   float64_is_any_nan(farg2.d))) {
> -        ret = 0x01UL;
> +        ret = CRF_SO;
>      } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> -        ret = 0x08UL;
> +        ret = CRF_LT;
>      } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> -        ret = 0x04UL;
> +        ret = CRF_GT;
>      } else {
> -        ret = 0x02UL;
> +        ret = CRF_EQ;
>      }
>  
>      env->fpscr &= ~(0x0F << FPSCR_FPRF);
> -    env->fpscr |= ret << FPSCR_FPRF;
> -    env->crf[crfD] = ret;
> -    if (unlikely(ret == 0x01UL
> +    env->fpscr |= (0x01 << FPSCR_FPRF) << ret;
> +    env->crf[crfD] = (1 << ret);
> +    if (unlikely(ret == CRF_SO
>                   && (float64_is_signaling_nan(farg1.d) ||
>                       float64_is_signaling_nan(farg2.d)))) {
>          /* sNaN comparison */
> @@ -1119,19 +1119,19 @@ void helper_fcmpo(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
>  
>      if (unlikely(float64_is_any_nan(farg1.d) ||
>                   float64_is_any_nan(farg2.d))) {
> -        ret = 0x01UL;
> +        ret = CRF_SO;
>      } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> -        ret = 0x08UL;
> +        ret = CRF_LT;
>      } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> -        ret = 0x04UL;
> +        ret = CRF_GT;
>      } else {
> -        ret = 0x02UL;
> +        ret = CRF_EQ;
>      }
>  
>      env->fpscr &= ~(0x0F << FPSCR_FPRF);
> -    env->fpscr |= ret << FPSCR_FPRF;
> -    env->crf[crfD] = ret;
> -    if (unlikely(ret == 0x01UL)) {
> +    env->fpscr |= (0x01 << FPSCR_FPRF) << ret;
> +    env->crf[crfD] = (1 << ret);
> +    if (unlikely(ret == CRF_SO)) {
>          if (float64_is_signaling_nan(farg1.d) ||
>              float64_is_signaling_nan(farg2.d)) {
>              /* sNaN comparison */
> 

I like this patch.

Nit: for the fcmp* functions, "ret" is not a very good name for the variable.  Since this is a cleanup patch, I would suggest renaming it to "fpcc".

Other than that ...

Reviewed-by: Tom Musta <tommusta@gmail.com>
Tested-by: Tom Musta <tommusta@gmail.com>