Re: [PATCH v2 1/6] target/arm/tcg: Implement new instructions for FPRCVT

["Alex Bennée" <alex.bennee@linaro.org>]

Jim MacArthur <jim.macarthur@linaro.org> writes:

> Adds the opcode format for the SIMD versions of FCVTXX and [US]CVTF.
> These use very similar logic to the FP-to-general and general-to-FP
> register versions which exist, but use another SIMD/FP register
> as source or destination. The source and destination size rules are
> slightly different.
>
> Signed-off-by: Jim MacArthur <jim.macarthur@linaro.org>
> ---
>  target/arm/cpu-features.h      |  5 +++
>  target/arm/tcg/a64.decode      | 15 +++++++++
>  target/arm/tcg/translate-a64.c | 71 +++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 83 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
> index a80b251589..aba95f4253 100644
> --- a/target/arm/cpu-features.h
> +++ b/target/arm/cpu-features.h
> @@ -1665,6 +1665,11 @@ static inline bool isar_feature_aa64_f8mm4(const ARMISARegisters *id)
>      return FIELD_EX64_IDREG(id, ID_AA64FPFR0, F8MM4);
>  }
>  
> +static inline bool isar_feature_aa64_fprcvt(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64_IDREG(id, ID_AA64ISAR3, FPRCVT);
> +}
> +
>  /*
>   * Combinations of feature tests, for ease of use with TRANS_FEAT.
>   */
> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
> index 28cd1faf61..5b6f156d08 100644
> --- a/target/arm/tcg/a64.decode
> +++ b/target/arm/tcg/a64.decode
> @@ -1456,6 +1456,21 @@ FCVTZU_g        . 0011110 .. 111001 000000 ..... .....  @icvt
>  FCVTAS_g        . 0011110 .. 100100 000000 ..... .....  @icvt
>  FCVTAU_g        . 0011110 .. 100101 000000 ..... .....  @icvt
>  
> +# Conversion between floating-point and integer (SIMD & FP)
> +SCVTF_simd      . 0011110 .. 111100 000000 ..... .....  @icvt
> +UCVTF_simd      . 0011110 .. 111101 000000 ..... .....  @icvt
> +
> +FCVTAS_g_simd   . 0011110 .. 111010 000000 ..... .....  @icvt
> +FCVTAU_g_simd   . 0011110 .. 111011 000000 ..... .....  @icvt
> +FCVTMS_g_simd   . 0011110 .. 110100 000000 ..... .....  @icvt
> +FCVTMU_g_simd   . 0011110 .. 110101 000000 ..... .....  @icvt
> +FCVTNS_g_simd   . 0011110 .. 101010 000000 ..... .....  @icvt
> +FCVTNU_g_simd   . 0011110 .. 101011 000000 ..... .....  @icvt
> +FCVTPS_g_simd   . 0011110 .. 110010 000000 ..... .....  @icvt
> +FCVTPU_g_simd   . 0011110 .. 110011 000000 ..... .....  @icvt
> +FCVTZS_g_simd   . 0011110 .. 110110 000000 ..... .....  @icvt
> +FCVTZU_g_simd   . 0011110 .. 110111 000000 ..... .....  @icvt
> +
>  FJCVTZS         0 0011110 01 111110 000000 ..... .....  @rr
>  
>  FMOV_ws         0 0011110 00 100110 000000 ..... .....  @rr
> diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
> index 227719ef25..07f985a5f0 100644
> --- a/target/arm/tcg/translate-a64.c
> +++ b/target/arm/tcg/translate-a64.c
> @@ -9905,12 +9905,14 @@ TRANS(SCVTF_g, do_cvtf_g, a, true)
>  TRANS(UCVTF_g, do_cvtf_g, a, false)
>  
>  /*
> - * [US]CVTF (vector), scalar version.
> - * Which sounds weird, but really just means input from fp register
> + * [US]CVTF (vector), scalar or SIMD version.
> + * Which sounds weird, but really just means input from FP/SIMD register
>   * instead of input from general register.  Input and output element
> - * size are always equal.
> + * size are always equal for the scalar version and different for the
> + * SIMD version.
>   */
> -static bool do_cvtf_f(DisasContext *s, arg_fcvt *a, bool is_signed)
> +static bool do_cvtf_f(DisasContext *s, arg_fcvt *a, MemOp src_mop_int,
> +                      bool is_signed)
>  {
>      TCGv_i64 tcg_int;
>      int check = fp_access_check_scalar_hsd(s, a->esz);
> @@ -9918,14 +9920,18 @@ static bool do_cvtf_f(DisasContext *s, arg_fcvt *a, bool is_signed)
>      if (check <= 0) {
>          return check == 0;
>      }
> -
>      tcg_int = tcg_temp_new_i64();
> -    read_vec_element(s, tcg_int, a->rn, 0, a->esz | (is_signed ? MO_SIGN : 0));
> +    read_vec_element(s, tcg_int, a->rn, 0,
> +                     src_mop_int | (is_signed ? MO_SIGN : 0));
>      return do_cvtf_scalar(s, a->esz, a->rd, a->shift, tcg_int, is_signed);
>  }
>  
> -TRANS(SCVTF_f, do_cvtf_f, a, true)
> -TRANS(UCVTF_f, do_cvtf_f, a, false)
> +TRANS(SCVTF_f, do_cvtf_f, a, a->esz, true)
> +TRANS(UCVTF_f, do_cvtf_f, a, a->esz, false)
> +TRANS_FEAT(SCVTF_simd, aa64_fprcvt, do_cvtf_f, a,
> +           a->sf ? MO_64 : MO_32, true)
> +TRANS_FEAT(UCVTF_simd, aa64_fprcvt, do_cvtf_f, a,
> +           a->sf ? MO_64 : MO_32, false)
>  
>  static void do_fcvt_scalar(DisasContext *s, MemOp out, MemOp esz,
>                             TCGv_i64 tcg_out, int shift, int rn,
> @@ -10044,6 +10050,34 @@ static bool do_fcvt_g(DisasContext *s, arg_fcvt *a,
>      return true;
>  }
>  
> +/*
> + * Floating point to int conversion, but puts the result
> + * in a SIMD register.
> + */
> +static bool do_fcvt_simd(DisasContext *s, arg_fcvt *a,
> +                      ARMFPRounding rmode, bool is_signed)
> +{
> +    TCGv_i64 tcg_int;
> +    int check = fp_access_check_scalar_hsd(s, a->esz);
> +

I did test with:

  g_assert(s->is_nonstreaming == false);

which shows trans_FAIL or aarch64_tr_translate_insn setting had done
whatever it was supposed to.

> +    if (check <= 0) {
> +        return check == 0;
> +    }
> +    /*
> +     * a->sf should specify destination size (64 bit or 32 bit)
> +     * a->esz specifies source size
> +     */
> +    tcg_int = tcg_temp_new_i64();
> +    do_fcvt_scalar(s, (a->sf ? MO_64 : MO_32) | (is_signed ? MO_SIGN : 0),
> +                   a->esz, tcg_int, a->shift, a->rn, rmode);
> +
> +    if (!s->fpcr_nep) {
> +        clear_vec(s, a->rd);
> +    }
> +    write_vec_element(s, tcg_int, a->rd, 0, (a->sf ? MO_64 : MO_32));
> +    return true;
> +}
> +
>  TRANS(FCVTNS_g, do_fcvt_g, a, FPROUNDING_TIEEVEN, true)
>  TRANS(FCVTNU_g, do_fcvt_g, a, FPROUNDING_TIEEVEN, false)
>  TRANS(FCVTPS_g, do_fcvt_g, a, FPROUNDING_POSINF, true)
> @@ -10055,6 +10089,27 @@ TRANS(FCVTZU_g, do_fcvt_g, a, FPROUNDING_ZERO, false)
>  TRANS(FCVTAS_g, do_fcvt_g, a, FPROUNDING_TIEAWAY, true)
>  TRANS(FCVTAU_g, do_fcvt_g, a, FPROUNDING_TIEAWAY, false)
>  
> +TRANS_FEAT(FCVTNS_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_TIEEVEN, true)
> +TRANS_FEAT(FCVTNU_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_TIEEVEN, false)
> +TRANS_FEAT(FCVTPS_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_POSINF, true)
> +TRANS_FEAT(FCVTPU_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_POSINF, false)
> +TRANS_FEAT(FCVTMS_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_NEGINF, true)
> +TRANS_FEAT(FCVTMU_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_NEGINF, false)
> +TRANS_FEAT(FCVTZS_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_ZERO, true)
> +TRANS_FEAT(FCVTZU_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_ZERO, false)
> +TRANS_FEAT(FCVTAS_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_TIEAWAY, true)
> +TRANS_FEAT(FCVTAU_g_simd, aa64_fprcvt, do_fcvt_simd, a,
> +           FPROUNDING_TIEAWAY, false)
> +
>  /*
>   * FCVT* (vector), scalar version.
>   * Which sounds weird, but really just means output to fp register

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro