Re: [PATCH v2 24/45] target/arm: Implement fp16 for Neon VRECPE, VRSQRTE using gvec

[Peter Maydell]

On Fri, 28 Aug 2020 at 21:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/28/20 11:33 AM, Peter Maydell wrote:
> > We already have gvec helpers for floating point VRECPE and
> > VRQSRTE, so convert the Neon decoder to use them and
> > add the fp16 support.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/arm/translate-neon.c.inc | 29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/arm/translate-neon.c.inc 
> > b/target/arm/translate-neon.c.inc
> > index 9d0959517fa..872f093a1fc 100644
> > --- a/target/arm/translate-neon.c.inc
> > +++ b/target/arm/translate-neon.c.inc
> > @@ -3857,13 +3857,38 @@ static bool do_2misc_fp(DisasContext *s, arg_2misc 
> > *a,
> >          return do_2misc_fp(s, a, FUNC);                         \
> >      }
> >
> > -DO_2MISC_FP(VRECPE_F, gen_helper_recpe_f32)
> > -DO_2MISC_FP(VRSQRTE_F, gen_helper_rsqrte_f32)
> >  DO_2MISC_FP(VCVT_FS, gen_helper_vfp_sitos)
> >  DO_2MISC_FP(VCVT_FU, gen_helper_vfp_uitos)
> >  DO_2MISC_FP(VCVT_SF, gen_helper_vfp_tosizs)
> >  DO_2MISC_FP(VCVT_UF, gen_helper_vfp_touizs)
> >
> > +#define DO_2MISC_FP_VEC(INSN, HFUNC, SFUNC)                             \
> > +    static void gen_##INSN(unsigned vece, uint32_t rd_ofs,              \
> > +                           uint32_t rm_ofs,                             \
> > +                           uint32_t oprsz, uint32_t maxsz)              \
> > +    {                                                                   \
> > +        static gen_helper_gvec_2_ptr * const fns[4] = {                 \
> > +            NULL, HFUNC, SFUNC, NULL,                                   \
> > +        };                                                              \
> > +        TCGv_ptr fpst;                                                  \
> > +        fpst = fpstatus_ptr(vece == 1 ? FPST_STD_F16 : FPST_STD);       \
>
> Perhaps clearer with MO_16 instead of 1.

OK (matches what we've mostly used elsewhere when selecting the
argument to fpstatus_ptr()).

> > +        tcg_gen_gvec_2_ptr(rd_ofs, rm_ofs, fpst, oprsz, maxsz, 0,       \
> > +                           fns[vece]);                                  \
> > +        tcg_temp_free_ptr(fpst);                                        \
> > +    }                                                                   \
> > +    static bool trans_##INSN(DisasContext *s, arg_2misc *a)             \
> > +    {                                                                   \
> > +        if (a->size == 0 ||                                             \
> > +            (a->size == 1 && !dc_isar_feature(aa32_fp16_arith, s)))     \
> > +        {                                                               \
>
> Likewise

Here I'm not so sure -- the thing you'd be cross-checking the size
against is the Arm ARM, which uses '01' and '10' (or '1' and '0', depending
on the insn), since it's the raw size field from the insn we're
looking at, not the data-type size. They happen to be the same values
for this insn, but aren't for all insns. (VADD (fp) and the other
@3same_fp patterns are examples of the other encoding.)

The other approach would be to standardize on "the decodetree pattern
always converts the size to the data-type size, regardless of how
it's encoded in the insn fields", and then you could check against
MO_16 here. Would that be better ?

> , and the { is on the wrong line.

Will fix.

thanks
-- PMM