Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Víctor Colombo]

On 05/09/2022 12:56, Richard Henderson wrote:
> On 9/5/22 13:37, Víctor Colombo wrote:
> > These two helpers are almost identical, differing only by the softfloat
> > operation it calls. Merge them into one using a macro.
> > Also, take this opportunity to capitalize the helper name as we moved
> > the instruction to decodetree in a previous patch.
> >
> > Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
> > ---
> >   target/ppc/fpu_helper.c            | 35 +++++++++++-------------------
> >   target/ppc/helper.h                |  4 ++--
> >   target/ppc/translate/fp-impl.c.inc |  4 ++--
> >   3 files changed, 17 insertions(+), 26 deletions(-)
> >
> > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> > index 0f045b70f8..32995179b5 100644
> > --- a/target/ppc/fpu_helper.c
> > +++ b/target/ppc/fpu_helper.c
> > @@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState 
> > *env, int flags,
> >       }
> >   }
> >
> > -/* fsqrt - fsqrt. */
> > -float64 helper_fsqrt(CPUPPCState *env, float64 arg)
> > -{
> > -    float64 ret = float64_sqrt(arg, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_sqrt(env, flags, 1, GETPC());
> > -    }
> > -
> > -    return ret;
> > +#define FPU_FSQRT(name, 
> > op)                                                   \
> > +float64 helper_##name(CPUPPCState *env, float64 
> > arg)                          \
> > +{                                                                             
> > \
> > +    float64 ret = op(arg, 
> > &env->fp_status);                                   \
> > +    int flags = 
> > get_float_exception_flags(&env->fp_status);                   \
> > +                                                                              
> > \
> > +    if (unlikely(flags & float_flag_invalid)) 
> > {                               \
> > +        float_invalid_op_sqrt(env, flags, 1, 
> > GETPC());                        \
> > +    
> > }                                                                         
> > \
>
> Existing bug, but this is missing to clear fp status to start.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> r~

Hello Richard, thanks for your review!
gen_reset_fpstatus() is called by the inline implementation in
do_helper_fsqrt() before calling the helper (patch 1).
It's probably better to move the call to inside the helper.

> > +                                                                              
> > \
> > +    return 
> > ret;                                                               \
> >   }
> >
> > -/* fsqrts - fsqrts. */
> > -float64 helper_fsqrts(CPUPPCState *env, float64 arg)
> > -{
> > -    float64 ret = float64r32_sqrt(arg, &env->fp_status);
> > -    int flags = get_float_exception_flags(&env->fp_status);
> > -
> > -    if (unlikely(flags & float_flag_invalid)) {
> > -        float_invalid_op_sqrt(env, flags, 1, GETPC());
> > -    }
> > -    return ret;
> > -}
> > +FPU_FSQRT(FSQRT, float64_sqrt)
> > +FPU_FSQRT(FSQRTS, float64r32_sqrt)
> >
> >   /* fre - fre. */
> >   float64 helper_fre(CPUPPCState *env, float64 arg)
> > diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> > index 159b352f6e..68610896b8 100644
> > --- a/target/ppc/helper.h
> > +++ b/target/ppc/helper.h
> > @@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64)
> >   DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64)
> >   DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64)
> >   DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64)
> > -DEF_HELPER_2(fsqrt, f64, env, f64)
> > -DEF_HELPER_2(fsqrts, f64, env, f64)
> > +DEF_HELPER_2(FSQRT, f64, env, f64)
> > +DEF_HELPER_2(FSQRTS, f64, env, f64)
> >   DEF_HELPER_2(fre, i64, env, i64)
> >   DEF_HELPER_2(fres, i64, env, i64)
> >   DEF_HELPER_2(frsqrte, i64, env, i64)
> > diff --git a/target/ppc/translate/fp-impl.c.inc 
> > b/target/ppc/translate/fp-impl.c.inc
> > index 7a90c0e350..8d5cf0f982 100644
> > --- a/target/ppc/translate/fp-impl.c.inc
> > +++ b/target/ppc/translate/fp-impl.c.inc
> > @@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, 
> > arg_A_tb *a,
> >       return true;
> >   }
> >
> > -TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);
> > -TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts);
> > +TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT);
> > +TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS);
> >
> >   /***                     Floating-Point 
> > multiply-and-add                   ***/
> >   /* fmadd - fmadds */


--
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>