Re: [PATCH v2 14/28] target/arm: Implement FPCXT_NS fp system register

[Richard Henderson]

On 11/19/20 3:56 PM, Peter Maydell wrote:
> Implement the v8.1M FPCXT_NS floating-point system register.  This is
> a little more complicated than FPCXT_S, because it has specific
> handling for "current FP state is inactive", and it only wants to do
> PreserveFPState(), not the full set of actions done by
> ExecuteFPCheck() which vfp_access_check() implements.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate-vfp.c.inc | 110 ++++++++++++++++++++++++++++++---
>  1 file changed, 103 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/translate-vfp.c.inc b/target/arm/translate-vfp.c.inc
> index ebc59daf613..1c2d31f6f30 100644
> --- a/target/arm/translate-vfp.c.inc
> +++ b/target/arm/translate-vfp.c.inc
> @@ -647,8 +647,20 @@ typedef enum fp_sysreg_check_result {
>      fp_sysreg_check_continue, /* caller should continue generating code */
>  } fp_sysreg_check_result;
>  
> -static fp_sysreg_check_result fp_sysreg_checks(DisasContext *s, int regno)
> +/*
> + * Emit code to check common UNDEF cases and handle lazy state preservation
> + * including the special casing for FPCXT_NS. For reads of sysregs, caller
> + * should provide storefn and opaque; for writes to sysregs these can be 
> NULL.
> + * On return, if *insn_end_label is not NULL the caller needs to 
> gen_set_label()
> + * it at the end of the other code generated for the insn.
> + */
> +static fp_sysreg_check_result fp_sysreg_checks(DisasContext *s, int regno,
> +                                               fp_sysreg_storefn *storefn,
> +                                               void *opaque,
> +                                               TCGLabel **insn_end_label)

I think it is really ugly to bury the fpAccess check in here.

> -    if (!vfp_access_check(s)) {
> -        return fp_sysreg_check_done;

I think it would be better to do

  if (regno != ARM_VFP_FPCXT_NS && !vfp_access_check(s))

and split out

> +        /* fpInactive = FPCCR_NS.ASPEN == 1 && CONTROL.FPCA == 0 */
> +        TCGLabel *fp_active_label = gen_new_label();
> +        TCGv_i32 aspen, fpca;
> +        aspen = load_cpu_field(v7m.fpccr[M_REG_NS]);
> +        fpca = load_cpu_field(v7m.control[M_REG_S]);
> +        tcg_gen_andi_i32(aspen, aspen, R_V7M_FPCCR_ASPEN_MASK);
> +        tcg_gen_xori_i32(aspen, aspen, R_V7M_FPCCR_ASPEN_MASK);
> +        tcg_gen_andi_i32(fpca, fpca, R_V7M_CONTROL_FPCA_MASK);
> +        tcg_gen_or_i32(fpca, fpca, aspen);
> +        tcg_gen_brcondi_i32(TCG_COND_NE, fpca, 0, fp_active_label);
> +        tcg_temp_free_i32(aspen);
> +        tcg_temp_free_i32(fpca);
> +
> +        /* fpInactive case: FPCXT_NS reads as FPDSCR_NS, write is NOP */
> +        if (storefn) {
> +            TCGv_i32 tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]);
> +            storefn(s, opaque, tmp);
> +        }
> +        /* jump to end of insn */
> +        *insn_end_label = gen_new_label();
> +        tcg_gen_br(*insn_end_label);
> +
> +        gen_set_label(fp_active_label);
> +        /* !fpInactive: PreserveFPState() and handle register as normal */
> +        gen_preserve_fp_state(s);

... all of this to a separate function.

Then VLDR becomes

   case ARM_VFP_FPCXT_NS:
        lab_inactive = gen_new_label();
        gen_branch_fpInactive(s, TCG_COND_NE, lab_inactive);
        ...
        gen_set_label(lab_inactive);
        gen_lookup_tb();
        break;

and VSTR becomes

   case ARM_VFP_FPCXT_NS:
       lab_end = gen_new_label();
       lab_active = gen_new_label();
       gen_branch_fpInactive(s, TCG_COND_EQ, lab_active);
       ...
       tcg_gen_br(lab_end);
       gen_set_label(lab_active);
       /* fall through */
   case ARM_VFP_FPCXT_S:
       ...
   }
   if (lab_end) {
       gen_set_label(lab_end);
   }


r~