Re: [PATCH v1 04/11] target/arm: Implement vector float32 to bfloat16 conversion

[Peter Maydell <peter.maydell@linaro.org>]

On Sat, 17 Apr 2021 at 01:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This is BFCVT{N,T} for both AArch64 AdvSIMD and SVE,
> and VCVT.BF16.F32 for AArch32 NEON.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> @@ -7567,6 +7568,7 @@ void HELPER(NAME)(void *vd, void *vn, void *vg, void *status, uint32_t desc)  \
>  }
>
>  DO_FCVTNT(sve2_fcvtnt_sh, uint32_t, uint16_t, H1_4, H1_2, sve_f32_to_f16)
> +DO_FCVTNT(sve_bfcvtnt,    uint32_t, uint16_t, H1_4, H1_2, float32_to_bfloat16)
>  DO_FCVTNT(sve2_fcvtnt_ds, uint64_t, uint32_t, H1_4, H1_2, float64_to_float32)

Not related to this patch, but are the H macros for sve2_fcvtnt_ds definitely
right? Just noticed they're the same as the ones being used for the f32->f16
helpers despite the types being different sizes.

> diff --git a/target/arm/translate-neon.c.inc b/target/arm/translate-neon.c.inc
> index f1893b1dc8..8cc53892d6 100644
> --- a/target/arm/translate-neon.c.inc
> +++ b/target/arm/translate-neon.c.inc
> @@ -3413,6 +3413,51 @@ static bool trans_VSHLL(DisasContext *s, arg_2misc *a)
>      return true;
>  }
>
> +static bool trans_VCVT_B16_F32(DisasContext *s, arg_2misc *a)
> +{
> +    TCGv_ptr fpst;
> +    TCGv_i64 tmp;
> +    TCGv_i32 dst0, dst1;
> +
> +    if (!dc_isar_feature(aa32_bf16, s)) {
> +        return false;
> +    }

Do we need to also check ARM_FEATURE_NEON here ?

> +
> +    /* UNDEF accesses to D16-D31 if they don't exist. */
> +    if (!dc_isar_feature(aa32_simd_r32, s) &&
> +        ((a->vd | a->vm) & 0x10)) {
> +        return false;
> +    }
> +
> +    if ((a->vm & 1) || (a->size != 1)) {
> +        return false;
> +    }
> +
> +    if (!vfp_access_check(s)) {
> +        return true;
> +    }
> +
> +    fpst = fpstatus_ptr(FPST_STD);
> +    tmp = tcg_temp_new_i64();
> +    dst0 = tcg_temp_new_i32();
> +    dst1 = tcg_temp_new_i32();
> +
> +    read_neon_element64(tmp, a->vm, 0, MO_64);
> +    gen_helper_bfcvt_pair(dst0, tmp, fpst);
> +
> +    read_neon_element64(tmp, a->vm, 1, MO_64);
> +    gen_helper_bfcvt_pair(dst1, tmp, fpst);
> +
> +    write_neon_element32(dst0, a->vd, 0, MO_32);
> +    write_neon_element32(dst1, a->vd, 1, MO_32);
> +
> +    tcg_temp_free_i64(tmp);
> +    tcg_temp_free_i32(dst0);
> +    tcg_temp_free_i32(dst1);
> +    tcg_temp_free_ptr(fpst);
> +    return true;
> +}
> +
>  static bool trans_VCVT_F16_F32(DisasContext *s, arg_2misc *a)
>  {
>      TCGv_ptr fpst;
> --
> 2.25.1

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM