Re: [PATCH 1/2] target/ppc: Fix FPSCR.FI bit being cleared when it shouldn't

[Richard Henderson <richard.henderson@linaro.org>]

On 5/9/22 07:48, Víctor Colombo wrote:
> -static inline void float_inexact_excp(CPUPPCState *env)
> +static inline void float_inexact_excp(CPUPPCState *env, bool set_fi)
>   {
>       CPUState *cs = env_cpu(env);
>   
> -    env->fpscr |= FP_FI;
> +    if (set_fi) {
> +        env->fpscr |= FP_FI;
> +    }
>       env->fpscr |= FP_XX;
>       /* Update the floating-point exception summary */
>       env->fpscr |= FP_FX;

I think it would be better to move the change to FI from here...

> @@ -462,7 +464,8 @@ void helper_fpscr_check_status(CPUPPCState *env)
>       }
>   }
>   
> -static void do_float_check_status(CPUPPCState *env, uintptr_t raddr)
> +static void do_float_check_status(CPUPPCState *env, bool change_fi,
> +                                  uintptr_t raddr)
>   {
>       CPUState *cs = env_cpu(env);
>       int status = get_float_exception_flags(&env->fp_status);
> @@ -473,8 +476,8 @@ static void do_float_check_status(CPUPPCState *env, uintptr_t raddr)
>           float_underflow_excp(env);
>       }
>       if (status & float_flag_inexact) {
> -        float_inexact_excp(env);
> -    } else {
> +        float_inexact_excp(env, change_fi);
> +    } else if (change_fi) {
>           env->fpscr &= ~FP_FI; /* clear the FPSCR[FI] bit */
>       }

... to here.  E.g.

     if (status & float_flag_inexact) {
         float_inexact_excp(env);
     }
     if (change_fi) {
         if (status & float_flag_inexact) {
             env->fpscr |= FP_FI;
         } else {
             env->fpscr &= ~FP_FI;
         }
     }

or indeed

     env->fpscr = FIELD_DP64(env->fpscr, FPSCR, FI,
                             !!(status & float_flag_inexact));

Otherwise it all looks plausible.

> @@ -1690,9 +1693,9 @@ uint32_t helper_efdcmpeq(CPUPPCState *env, uint64_t op1, uint64_t op2)
>    *   nels  - number of elements (1, 2 or 4)
>    *   tp    - type (float32 or float64)
>    *   fld   - vsr_t field (VsrD(*) or VsrW(*))
> - *   sfprf - set FPRF
> + *   sfifprf - set FI and FPRF
>    */
> -#define VSX_ADD_SUB(name, op, nels, tp, fld, sfprf, r2sp)                    \
> +#define VSX_ADD_SUB(name, op, nels, tp, fld, sfifprf, r2sp)                  \


It might be easier to read if this renaming is done as a separate step.


r~