Re: [PATCH v2 12/12] target/ppc: Use gvec to decode XVTSTDC[DS]P

[Lucas Mateus Martins Araujo e Castro]

On 10/10/2022 16:42, Richard Henderson wrote:
>  
> On 10/10/22 12:13, Lucas Mateus Castro(alqotel) wrote:
> > +/* test if +Inf or -Inf */
> > +static void gen_is_any_inf(unsigned vece, TCGv_vec t, TCGv_vec b)
> > +{
> > +    uint64_t exp_msk = (vece == MO_32) ? (uint32_t)EXP_MASK_SP : 
> > EXP_MASK_DP;
> > +    uint64_t sgn_msk = (vece == MO_32) ? (uint32_t)SGN_MASK_SP : 
> > SGN_MASK_DP;
> > +    tcg_gen_andc_vec(vece, b, b, tcg_constant_vec_matching(t, vece, 
> > exp_msk));
> > +    tcg_gen_cmp_vec(TCG_COND_EQ, vece, t, b,
> > +                    tcg_constant_vec_matching(t, vece, sgn_msk));
> > +}
>
> Should be clearing sign and comparing exp, not the other way.
Yeah this was a mistake, I'll fix it in the next version.
Kind of weird that my tests didn't caught this, probably should test 
that the '.out' risu file I'm using actually test every immediate value.
> > +    GVecGen2 op = {
> > +        .fno = (vece == MO_32) ? gen_helper_XVTSTDCSP : 
> > gen_helper_XVTSTDCDP,
> > +        .vece = vece,
> > +        .opt_opc = vecop_list
> >       };
> >
> >       REQUIRE_VSX(ctx);
> >
> > -    tcg_gen_gvec_2i(vsr_full_offset(a->xt), vsr_full_offset(a->xb),
> > -                    16, 16, (int32_t)(a->uim), &op[vece - MO_32]);
> > +    switch (a->uim) {
> > +    case 0:
> > +        set_cpu_vsr(a->xt, tcg_constant_i64(0), true);
> > +        set_cpu_vsr(a->xt, tcg_constant_i64(0), false);
> > +        break;
> > +    case ((1 << 0) | (1 << 1)):
> > +        /* test if +Denormal or -Denormal */
> > +        op.fniv = gen_is_any_denormal,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
>
> This default setting of .fno doesn't work, because the helper requires 
> simd_data set,
> which this invocation via tcg_gen_gvec_2 will not provide.
>
> You could fix this by using GVecGen2i and tcg_gen_gvec_2i, and ignoring 
> the immediate
And I can remove the new #include from int_helper which was bothering me.
> argument added to the functions above.  Which also means...
>
> > +    case (1 << 0):
> > +        /* test if -Denormal */
> > +        op.fniv = gen_is_neg_denormal,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    case (1 << 1):
> > +        /* test if +Denormal */
> > +        op.fniv = gen_is_pos_denormal,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    case ((1 << 2) | (1 << 3)):
> > +        /* test if +0 or -0 */
> > +        op.fniv = gen_is_any_zero,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    case (1 << 2):
> > +        /* test if -0 */
> > +        op.fniv = gen_is_neg_zero,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    case (1 << 3):
> > +        /* test if +0 */
> > +        op.fniv = gen_is_pos_zero,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    case ((1 << 4) | (1 << 5)):
> > +        /* test if +Inf or -Inf */
> > +        op.fniv = gen_is_any_inf,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    case (1 << 4):
> > +        /* test if -Inf */
> > +        op.fniv = gen_is_neg_inf,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    case (1 << 5):
> > +        /* test if +Inf */
> > +        op.fniv = gen_is_pos_inf,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    case (1 << 6):
> > +        /* test if NaN */
> > +        op.fniv = gen_is_nan,
> > +        tcg_gen_gvec_2(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16, 16,
> > +                       &op);
> > +        break;
> > +    default:
> > +        tcg_gen_gvec_2_ool(vsr_full_offset(a->xt), 
> > vsr_full_offset(a->xb), 16,
> > +                           16, (int32_t)(a->uim), op.fno);
> > +        break;
>
> You can have only the store to op.fniv in the switch, remove the 
> default, and have a
> common call to tcg_gen_gvec_2i after the switch.
>
>
> r~
I'll send a v3 with these changes.
--
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Junior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>