Re: [PATCH 11/19] target/arm: Replace ARM_FEATURE_VFP4 with isar_feature_aa32_simdfmac

[Peter Maydell]

On Thu, 20 Feb 2020 at 16:37, Peter Maydell <address@hidden> wrote:
>
> On Fri, 14 Feb 2020 at 18:16, Richard Henderson
> <address@hidden> wrote:
> >
> > All remaining tests for VFP4 are for fused multiply-add insns.
> >
> > Signed-off-by: Richard Henderson <address@hidden>
> > ---
> >  target/arm/cpu.h               |  5 +++++
> >  target/arm/translate-vfp.inc.c | 12 ++++++++----
> >  target/arm/translate.c         |  2 +-
> >  3 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 4ff28418df..f27b8e35df 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -3468,6 +3468,11 @@ static inline bool 
> > isar_feature_aa32_fp16_dpconv(const ARMISARegisters *id)
> >      return FIELD_EX32(id->mvfr1, MVFR1, FPHP) > 1;
> >  }
> >
> > +static inline bool isar_feature_aa32_simdfmac(const ARMISARegisters *id)
> > +{
> > +    return FIELD_EX32(id->mvfr1, MVFR1, SIMDFMAC) != 0;
> > +}
>
> This is tricky, because the SIMDFMAC register
> field indicates "do we have fused-multiply-accumulate
> for either VFP or Neon", so in a VFP-no-Neon core or
> a Neon-no-VFP core it will be 1 but can't be used on its
> own as a gate on "should this insn be present".
>
> Currently in the part of arm_cpu_realize() which handles
> the user having selected vfp=off and/or neon=off we
> do allow (for AArch32 cores) both of those combinations.
>
> trans_VFM_dp already tests aa32_fpdp_v2, so I think the
> main thing we need to do is add a test on aa32_fpsp_v2 to
> trans_VFM_sp.
>
> We clear the SIMDFMAC field to 0 in the !has_neon condition,
> and I think that should actually be in the !neon && !vfp part.
>
> I propose to squash in the following and beef up the commit message:
>

> +        /* Despite the name, this field covers both VFP and Neon */
> +        u = cpu->isar.mvfr1;
> +        u = FIELD_DP32(u, MVFR1, SIMDFMAC, 0);
> +        cpu->isar.mvfr1;

 ... "cpu->isar.mvfr1 = u;", obviously.


> -    if (!dc_isar_feature(aa32_simdfmac, s)) {
> +    if (!dc_isar_feature(aa32_simdfmac, s) ||
> +        !dc_isar_feature(aa32_fpsp_v2, s)) {
> +        return false;
> +    }

and not that extra "}".

-- PMM