Re: [PATCH v4 38/47] target/ppc: Refactor VSX_SCALAR_CMP_DP

[Víctor Colombo]

On 22/02/2022 21:20, Richard Henderson wrote:> On 2/22/22 04:36, 
matheus.ferst@eldorado.org.br wrote:
> > From: Víctor Colombo <victor.colombo@eldorado.org.br>
> >
> > Refactor VSX_SCALAR_CMP_DP, changing its name to VSX_SCALAR_CMP and
> > prepare the helper to be used for quadword comparisons.
> >
> > Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
> > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> > ---
> >   target/ppc/fpu_helper.c | 31 ++++++++++++++-----------------
> >   1 file changed, 14 insertions(+), 17 deletions(-)
> >
> > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> > index 9b034d1fe4..5ebbcfe3b7 100644
> > --- a/target/ppc/fpu_helper.c
> > +++ b/target/ppc/fpu_helper.c
> > @@ -2265,28 +2265,30 @@ VSX_MADDQ(XSNMSUBQP, NMSUB_FLGS, 0)
> >   VSX_MADDQ(XSNMSUBQPO, NMSUB_FLGS, 0)
> >
> >   /*
> > - * VSX_SCALAR_CMP_DP - VSX scalar floating point compare double 
> > precision
> > + * VSX_SCALAR_CMP - VSX scalar floating point compare
> >    *   op    - instruction mnemonic
> > + *   tp    - type
> >    *   cmp   - comparison operation
> >    *   exp   - expected result of comparison
> > + *   fld   - vsr_t field
> >    *   svxvc - set VXVC bit
> >    */
> > -#define VSX_SCALAR_CMP_DP(op, cmp, exp, 
> > svxvc)                                \
> > +#define VSX_SCALAR_CMP(op, tp, cmp, fld, exp, 
> > svxvc)                          \
> >   void helper_##op(CPUPPCState *env, ppc_vsr_t 
> > *xt,                             \
> >                    ppc_vsr_t *xa, ppc_vsr_t 
> > *xb)                                \
> >   
> > {                                                                             
> > \
> > -    ppc_vsr_t t = 
> > *xt;                                                        \
> > +    ppc_vsr_t t = { 
> > };                                                        \
> >       bool vxsnan_flag = false, vxvc_flag = false, vex_flag = 
> > false;            \
> >                                                                                 
> > \
> > -    if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) 
> > ||             \
> > -        float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) 
> > {             \
> > +    if (tp##_is_signaling_nan(xa->fld, &env->fp_status) 
> > ||                    \
> > +        tp##_is_signaling_nan(xb->fld, &env->fp_status)) 
> > {                    \
> >           vxsnan_flag = 
> > true;                                                   \
> >           if (fpscr_ve == 0 && svxvc) 
> > {                                         \
> >               vxvc_flag = 
> > true;                                                 \
> >           
> > }                                                                     \
> >       } else if (svxvc) 
> > {                                                       \
> > -        vxvc_flag = float64_is_quiet_nan(xa->VsrD(0), 
> > &env->fp_status) ||     \
> > -            float64_is_quiet_nan(xb->VsrD(0), 
> > &env->fp_status);               \
> > +        vxvc_flag = tp##_is_quiet_nan(xa->fld, &env->fp_status) 
> > ||            \
> > +            tp##_is_quiet_nan(xb->fld, 
> > &env->fp_status);                      \
> >       }
>
> Note that this can be simplified further, using the full FloatRelation 
> result and
> float_flag_invalid_snan.
>
> Note that do_scalar_cmp gets half-way there, only checking for NaNs once 
> we have
> float_relation_unordered as a comparision result.  But it could go 
> further and check
> float_flag_invalid_snan and drop all of the other checks vs snan and qnan.
>
>
> r~

Hello Richard! Thanks for your review

Could you please elaborate more on how do you think using
float*_compare and its FloatRelation result would work here?
I noticed do_scalar_cmp modifies CR and sets FPCC flag, which
is not what VSX_SCALAR_CMP do. Using that function would require a
rework.

An option I though would be to bring into VSX_SCALAR_CMP the
important necessary parts, something like this:

#define VSX_SCALAR_CMP(op, tp, cmp, fld, svxvc, expr) 
      ...
    r = tp##_compare(xa->fld, xb->fld, &env->fp_status); 
       \
    if (expr) { 
       \
        memset(&t.fld, 0xFF, sizeof(t.fld)); 
       \
    } else if (r == float_relation_unordered) { 
       \
        if (env->fp_status.float_exception_flags & 
float_flag_invalid_snan) { \
            float_invalid_op_vxsnan(env, GETPC()); 
       \
            if (fpscr_ve == 0 && svxvc) { 
       \
                float_invalid_op_vxvc(env, 0, GETPC()); 
       \
            } 
       \
        } else if (svxvc) { 
       \
            if (tp##_is_quiet_nan(xa->fld, &env->fp_status) || 
       \
                tp##_is_quiet_nan(xb->fld, &env->fp_status)) { 
       \
                    float_invalid_op_vxvc(env, 0, GETPC()); 
       \
                } 
       \
        } 
       \
    } 
       \
...
VSX_SCALAR_CMP(XSCMPEQDP, float64, eq, VsrD(0), 0, r == 
float_relation_equal)
VSX_SCALAR_CMP(XSCMPGEDP, float64, le, VsrD(0), 1, \
    r == float_relation_equal || r == float_relation_greater)
VSX_SCALAR_CMP(XSCMPGTDP, float64, lt, VsrD(0), 1, r == 
float_relation_greater)

But this still looks convoluted. Another option I came with would be:

    ppc_vsr_t t = { }; 
       \

       \
    helper_reset_fpstatus(env); 
       \

       \
    if (tp##_##cmp##_quiet(xb->fld, xa->fld, &env->fp_status)) { 
       \
        memset(&t.fld, 0xFF, sizeof(t.fld)); 
       \
    } 
       \

       \
    if (env->fp_status.float_exception_flags & float_flag_invalid_snan) 
{     \
        float_invalid_op_vxsnan(env, GETPC()); 
       \
        if (fpscr_ve == 0 && svxvc) { 
       \
            float_invalid_op_vxvc(env, 0, GETPC()); 
       \
        } 
       \
    } else if (svxvc) { 
       \
        if (tp##_is_quiet_nan(xa->fld, &env->fp_status) || 
       \
            tp##_is_quiet_nan(xb->fld, &env->fp_status)) { 
       \
                float_invalid_op_vxvc(env, 0, GETPC()); 
       \
            } 
       \
    } 
       \

Is this close to what you were thinking?

Thank you very much!

-- Víctor