Re: [PATCH 23/67] target/arm: Convert FMOV, FABS, FNEG (scalar) to decodetree

[Peter Maydell]

On Sun, 1 Dec 2024 at 15:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/tcg/translate-a64.c | 104 ++++++++++++++++++++++-----------
>  target/arm/tcg/a64.decode      |   7 +++
>  2 files changed, 78 insertions(+), 33 deletions(-)
>
> diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
> index 2d3566e45c..87731e0be4 100644
> --- a/target/arm/tcg/translate-a64.c
> +++ b/target/arm/tcg/translate-a64.c
> @@ -8287,6 +8287,67 @@ static bool trans_CSEL(DisasContext *s, arg_CSEL *a)
>      return true;
>  }
>
> +typedef struct FPScalar1Int {
> +    void (*gen_h)(TCGv_i32, TCGv_i32);
> +    void (*gen_s)(TCGv_i32, TCGv_i32);
> +    void (*gen_d)(TCGv_i64, TCGv_i64);
> +} FPScalar1Int;
> +
> +static bool do_fp1_scalar_int(DisasContext *s, arg_rr_e *a,
> +                              const FPScalar1Int *f)
> +{
> +    switch (a->esz) {
> +    case MO_64:
> +        if (fp_access_check(s)) {
> +            TCGv_i64 t = read_fp_dreg(s, a->rn);
> +            f->gen_d(t, t);
> +            write_fp_dreg(s, a->rd, t);
> +        }
> +        break;
> +    case MO_32:
> +        if (fp_access_check(s)) {
> +            TCGv_i32 t = read_fp_sreg(s, a->rn);
> +            f->gen_s(t, t);
> +            write_fp_sreg(s, a->rd, t);
> +        }
> +        break;
> +    case MO_16:
> +        if (!dc_isar_feature(aa64_fp16, s)) {
> +            return false;
> +        }
> +        if (fp_access_check(s)) {
> +            TCGv_i32 t = read_fp_hreg(s, a->rn);
> +            f->gen_h(t, t);
> +            write_fp_sreg(s, a->rd, t);
> +        }
> +        break;
> +    default:
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static const FPScalar1Int f_scalar_fmov = {
> +    tcg_gen_mov_i32,
> +    tcg_gen_mov_i32,
> +    tcg_gen_mov_i64,
> +};
> +TRANS(FMOV_s, do_fp1_scalar_int, a, &f_scalar_fmov)
> +
> +static const FPScalar1Int f_scalar_fabs = {
> +    gen_vfp_absh,
> +    gen_vfp_abss,
> +    gen_vfp_absd,
> +};
> +TRANS(FABS_s, do_fp1_scalar_int, a, &f_scalar_fabs)
> +
> +static const FPScalar1Int f_scalar_fneg = {
> +    gen_vfp_negh,
> +    gen_vfp_negs,
> +    gen_vfp_negd,
> +};
> +TRANS(FNEG_s, do_fp1_scalar_int, a, &f_scalar_fneg)
> +
>  /* Floating-point data-processing (1 source) - half precision */
>  static void handle_fp_1src_half(DisasContext *s, int opcode, int rd, int rn)
>  {
> @@ -8295,15 +8356,6 @@ static void handle_fp_1src_half(DisasContext *s, int 
> opcode, int rd, int rn)
>      TCGv_i32 tcg_res = tcg_temp_new_i32();
>
>      switch (opcode) {
> -    case 0x0: /* FMOV */
> -        tcg_gen_mov_i32(tcg_res, tcg_op);
> -        break;
> -    case 0x1: /* FABS */
> -        gen_vfp_absh(tcg_res, tcg_op);
> -        break;
> -    case 0x2: /* FNEG */
> -        gen_vfp_negh(tcg_res, tcg_op);
> -        break;
>      case 0x3: /* FSQRT */
>          fpst = fpstatus_ptr(FPST_FPCR_F16);
>          gen_helper_sqrt_f16(tcg_res, tcg_op, fpst);
> @@ -8331,6 +8383,9 @@ static void handle_fp_1src_half(DisasContext *s, int 
> opcode, int rd, int rn)
>          gen_helper_advsimd_rinth(tcg_res, tcg_op, fpst);
>          break;
>      default:
> +    case 0x0: /* FMOV */
> +    case 0x1: /* FABS */
> +    case 0x2: /* FNEG */
>          g_assert_not_reached();
>      }
>

In these changes to the "handle this op" functions we make the
function assert if it's passed an op we've converted. But shouldn't
there also be a change which makes the calling function disas_fp_1src()
call unallocated_encoding() for the ops ?

> @@ -8349,15 +8404,6 @@ static void handle_fp_1src_single(DisasContext *s, int 
> opcode, int rd, int rn)
>      tcg_res = tcg_temp_new_i32();
>
>      switch (opcode) {
> -    case 0x0: /* FMOV */
> -        tcg_gen_mov_i32(tcg_res, tcg_op);
> -        goto done;
> -    case 0x1: /* FABS */
> -        gen_vfp_abss(tcg_res, tcg_op);
> -        goto done;
> -    case 0x2: /* FNEG */
> -        gen_vfp_negs(tcg_res, tcg_op);
> -        goto done;
>      case 0x3: /* FSQRT */
>          gen_helper_vfp_sqrts(tcg_res, tcg_op, tcg_env);
>          goto done;
> @@ -8393,6 +8439,9 @@ static void handle_fp_1src_single(DisasContext *s, int 
> opcode, int rd, int rn)
>          gen_fpst = gen_helper_frint64_s;
>          break;
>      default:
> +    case 0x0: /* FMOV */
> +    case 0x1: /* FABS */
> +    case 0x2: /* FNEG */
>          g_assert_not_reached();
>      }
>
> @@ -8417,22 +8466,10 @@ static void handle_fp_1src_double(DisasContext *s, 
> int opcode, int rd, int rn)
>      TCGv_ptr fpst;
>      int rmode = -1;
>
> -    switch (opcode) {
> -    case 0x0: /* FMOV */
> -        gen_gvec_fn2(s, false, rd, rn, tcg_gen_gvec_mov, 0);
> -        return;
> -    }
> -
>      tcg_op = read_fp_dreg(s, rn);
>      tcg_res = tcg_temp_new_i64();
>
>      switch (opcode) {
> -    case 0x1: /* FABS */
> -        gen_vfp_absd(tcg_res, tcg_op);
> -        goto done;
> -    case 0x2: /* FNEG */
> -        gen_vfp_negd(tcg_res, tcg_op);
> -        goto done;
>      case 0x3: /* FSQRT */
>          gen_helper_vfp_sqrtd(tcg_res, tcg_op, tcg_env);
>          goto done;
> @@ -8465,6 +8502,9 @@ static void handle_fp_1src_double(DisasContext *s, int 
> opcode, int rd, int rn)
>          gen_fpst = gen_helper_frint64_d;
>          break;
>      default:
> +    case 0x0: /* FMOV */
> +    case 0x1: /* FABS */
> +    case 0x2: /* FNEG */
>          g_assert_not_reached();
>      }
>
> @@ -10881,13 +10921,11 @@ static void 
> disas_simd_two_reg_misc_fp16(DisasContext *s, uint32_t insn)
>          case 0x7b: /* FCVTZU */
>              gen_helper_advsimd_f16touinth(tcg_res, tcg_op, tcg_fpstatus);
>              break;
> -        case 0x6f: /* FNEG */
> -            tcg_gen_xori_i32(tcg_res, tcg_op, 0x8000);
> -            break;
>          case 0x7d: /* FRSQRTE */
>              gen_helper_rsqrte_f16(tcg_res, tcg_op, tcg_fpstatus);
>              break;
>          default:
> +        case 0x6f: /* FNEG */
>              g_assert_not_reached();
>          }

What's this change about? This bit of decode is not in the area that
corresponds to the new patterns you add to a64.decode.

Is this a bug in our existing decode where we decode something that
should be undef? I think that 0x6f here corresponds to the decode
table in section C4.1.96.6 ("Advanced SIMD scalar two-register
miscellaneous FP16"), where it is U:a:opcode == 1 1 01111. But
in that table that encoding is marked unallocated. A similar
thing is true for case 0x2f FABS and case 07f FSQRT. All three
of these should have set only_in_vector to true and not had the
code handling in the is_scalar branch of the function.

We should fix that bug in a separate patch before we do the
decodetree conversion, I think.

> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
> index 928e69da69..fca64c63c3 100644
> --- a/target/arm/tcg/a64.decode
> +++ b/target/arm/tcg/a64.decode
> @@ -47,6 +47,7 @@
>  @rr_h           ........ ... ..... ...... rn:5 rd:5     &rr_e esz=1
>  @rr_d           ........ ... ..... ...... rn:5 rd:5     &rr_e esz=3
>  @rr_sd          ........ ... ..... ...... rn:5 rd:5     &rr_e esz=%esz_sd
> +@rr_hsd         ........ ... ..... ...... rn:5 rd:5     &rr_e esz=%esz_hsd
>
>  @rrr_b          ........ ... rm:5 ...... rn:5 rd:5      &rrr_e esz=0
>  @rrr_h          ........ ... rm:5 ...... rn:5 rd:5      &rrr_e esz=1
> @@ -1321,6 +1322,12 @@ FMAXV_s         0110 1110 00 11000 01111 10 ..... 
> .....     @rr_q1e2
>  FMINV_h         0.00 1110 10 11000 01111 10 ..... .....     @qrr_h
>  FMINV_s         0110 1110 10 11000 01111 10 ..... .....     @rr_q1e2
>
> +# Floating-point data processing (1 source)
> +
> +FMOV_s          00011110 .. 1 000000 10000 ..... .....      @rr_hsd
> +FABS_s          00011110 .. 1 000001 10000 ..... .....      @rr_hsd
> +FNEG_s          00011110 .. 1 000010 10000 ..... .....      @rr_hsd
> +
>  # Floating-point Immediate
>
>  FMOVI_s         0001 1110 .. 1 imm:8 100 00000 rd:5         esz=%esz_hsd
> --
> 2.43.0

thanks
-- PMM