Re: [PATCH] target/ppc: Move floating-point arithmetic instructions to decodetree.

[Richard Henderson]

On 3/11/24 23:36, Nicholas Piggin wrote:
> On Thu Mar 7, 2024 at 9:03 PM AEST, Chinmay Rath wrote:
> > This patch moves the below instructions to decodetree specification :
> >
> >      f{add, sub, mul, div, re, rsqrte, madd, msub, nmadd, nmsub}[s][.] : A-form
> >      ft{div, sqrt}                                                     : X-form
> >
> > With this patch, all the floating-point arithmetic instructions have been
> > moved to decodetree.
> > The patch also merges the definitions of different sets of helper methods of
> > the above instructions, which are similar, using macros.
> > The changes were verified by validating that the tcg ops generated by those
> > instructions remain the same, which were captured with the '-d in_asm,op' flag.
> >
> > Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
>
> Hi Chinmay,
>
> Nice patch. I think the fpu_helper.c change should be done separately.
>
> > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> > index 4b3dcad5d1..a3fcfb3907 100644
> > --- a/target/ppc/fpu_helper.c
> > +++ b/target/ppc/fpu_helper.c
> > @@ -490,54 +490,16 @@ static void float_invalid_op_addsub(CPUPPCState *env, int 
> > flags,
> >       }
> >   }
> >   
> > -/* fadd - fadd. */
> > -float64 helper_fadd(CPUPPCState *env, float64 arg1, float64 arg2)
> > -{
> > -    float64 ret = float64_add(arg1, arg2, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_addsub(env, flags, 1, GETPC());
> > -    }
> > -
> > -    return ret;
> > -}
> > -
> > -/* fadds - fadds. */
> > -float64 helper_fadds(CPUPPCState *env, float64 arg1, float64 arg2)
> > -{
> > -    float64 ret = float64r32_add(arg1, arg2, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_addsub(env, flags, 1, GETPC());
> > -    }
> > -    return ret;
> > -}
> > -
> > -/* fsub - fsub. */
> > -float64 helper_fsub(CPUPPCState *env, float64 arg1, float64 arg2)
> > -{
> > -    float64 ret = float64_sub(arg1, arg2, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_addsub(env, flags, 1, GETPC());
> > -    }
> > -
> > -    return ret;
> > -}
> > -
> > -/* fsubs - fsubs. */
> > -float64 helper_fsubs(CPUPPCState *env, float64 arg1, float64 arg2)
> > -{
> > -    float64 ret = float64r32_sub(arg1, arg2, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_addsub(env, flags, 1, GETPC());
> > -    }
> > -    return ret;
> > +#define FPU_ADD_SUB(name, op)                                                 \
> > +float64 helper_##name(CPUPPCState *env, float64 arg1, float64 arg2)           \
> > +{                                                                             \
> > +    float64 ret = op(arg1, arg2, &env->fp_status);                            \
> > +    int flags = get_float_exception_flags(&env->fp_status);                   \
> > +                                                                              \
> > +    if (unlikely(flags & float_flag_invalid)) {                               \
> > +        float_invalid_op_addsub(env, flags, 1, GETPC());                      \
> > +    }                                                                         \
> > +    return ret;                                                               \
> >   }
> >   
> >   static void float_invalid_op_mul(CPUPPCState *env, int flags,
> > @@ -550,29 +512,17 @@ static void float_invalid_op_mul(CPUPPCState *env, int 
> > flags,
> >       }
> >   }
> >   
> > -/* fmul - fmul. */
> > -float64 helper_fmul(CPUPPCState *env, float64 arg1, float64 arg2)
> > -{
> > -    float64 ret = float64_mul(arg1, arg2, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_mul(env, flags, 1, GETPC());
> > -    }
> > -
> > -    return ret;
> > -}
> > -
> > -/* fmuls - fmuls. */
> > -float64 helper_fmuls(CPUPPCState *env, float64 arg1, float64 arg2)
> > -{
> > -    float64 ret = float64r32_mul(arg1, arg2, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_mul(env, flags, 1, GETPC());
> > -    }
> > -    return ret;
> > +#define FPU_MUL(name, op)                                                     \
> > +float64 helper_##name(CPUPPCState *env, float64 arg1, float64 arg2)           \
> > +{                                                                             \
> > +    float64 ret = op(arg1, arg2, &env->fp_status);                            \
> > +    int flags = get_float_exception_flags(&env->fp_status);                   \
> > +                                                                              \
> > +    if (unlikely(flags & float_flag_invalid)) {                               \
> > +        float_invalid_op_mul(env, flags, 1, GETPC());                         \
> > +    }                                                                         \
> > +                                                                              \
> > +    return ret;                                                               \
> >   }
> >   
> >   static void float_invalid_op_div(CPUPPCState *env, int flags,
> > @@ -587,37 +537,31 @@ static void float_invalid_op_div(CPUPPCState *env, int 
> > flags,
> >       }
> >   }
> >   
> > -/* fdiv - fdiv. */
> > -float64 helper_fdiv(CPUPPCState *env, float64 arg1, float64 arg2)
> > -{
> > -    float64 ret = float64_div(arg1, arg2, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_div(env, flags, 1, GETPC());
> > -    }
> > -    if (unlikely(flags & float_flag_divbyzero)) {
> > -        float_zero_divide_excp(env, GETPC());
> > -    }
> > -
> > -    return ret;
> > +#define FPU_DIV(name, op)                                                     \
> > +float64 helper_##name(CPUPPCState *env, float64 arg1, float64 arg2)           \
> > +{                                                                             \
> > +    float64 ret = op(arg1, arg2, &env->fp_status);                            \
> > +    int flags = get_float_exception_flags(&env->fp_status);                   \
> > +                                                                              \
> > +    if (unlikely(flags & float_flag_invalid)) {                               \
> > +        float_invalid_op_div(env, flags, 1, GETPC());                         \
> > +    }                                                                         \
> > +    if (unlikely(flags & float_flag_divbyzero)) {                             \
> > +        float_zero_divide_excp(env, GETPC());                                 \
> > +    }                                                                         \
> > +                                                                              \
> > +    return ret;                                                               \
> >   }
> >   
> > -/* fdivs - fdivs. */
> > -float64 helper_fdivs(CPUPPCState *env, float64 arg1, float64 arg2)
> > -{
> > -    float64 ret = float64r32_div(arg1, arg2, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_div(env, flags, 1, GETPC());
> > -    }
> > -    if (unlikely(flags & float_flag_divbyzero)) {
> > -        float_zero_divide_excp(env, GETPC());
> > -    }
> >   
> > -    return ret;
> > -}
> > +FPU_ADD_SUB(FADD, float64_add)
> > +FPU_ADD_SUB(FADDS, float64r32_add)
> > +FPU_ADD_SUB(FSUB, float64_sub)
> > +FPU_ADD_SUB(FSUBS, float64r32_sub)
> > +FPU_MUL(FMUL, float64_mul)
> > +FPU_MUL(FMULS, float64r32_mul)
> > +FPU_DIV(FDIV, float64_div)
> > +FPU_DIV(FDIVS, float64r32_div)
>
> Several of the existing macros have slightly different styles and
> patterns. FPU_FMADD being one. You're mostly adding an existing style
> and being pretty consistent so that's fine. It would be nice if the
> others could be made more regular, but maybe not feasible.
>
> There is also quite a bit of duplication in the templates just to
> cater to different error handling. I wonder if you could consolidate
> it a bit or make it nicer if you passed in the flags handling
> function as an argument. Then you could have:
>
>
> #define FPU_HELPER(name, op, flags_handler)                           \
> float64 helper_##name(CPUPPCState *env, float64 arg1, float64 arg2)   \
> {                                                                     \
>      float64 ret = op(arg1, arg2, &env->fp_status);                    \
>      int flags = get_float_exception_flags(&env->fp_status);           \
>      flags_handler(env, flags)                                         \
>      return ret;                                                       \
> }
>
> static inline void addsub_flags_handler(CPUPPCState *env, int flags)
> {
>      if (unlikely(flags & float_flag_invalid)) {
>          float_invalid_op_addsub(env, flags, 1, GETPC());
>      }
> }
>
> static inline void mul_flags_handler(CPUPPCState *env, int flags)
> {
>      if (unlikely(flags & float_flag_invalid)) {
>          float_invalid_op_mul(env, flags, 1, GETPC());
>      }
> }
>
> static inline void div_flags_handler(CPUPPCState *env, int flags)
> {
>      if (unlikely(flags & float_flag_invalid)) {
>          float_invalid_op_div(env, flags, 1, GETPC());
>      }
>      if (unlikely(flags & float_flag_divbyzero)) {
>          float_zero_divide_excp(env, GETPC());
>      }
> }

Beware -- GETPC() may only be called from the outermost helper.


r~