On Tue, 25 Jun 2024 at 06:09, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> target/arm/tcg/a64.decode | 5 +
> target/arm/tcg/translate-a64.c | 241 ++++++++++-----------------------
> 2 files changed, 76 insertions(+), 170 deletions(-)
>
> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
> index f330919851..4b2a6ba302 100644
> --- a/target/arm/tcg/a64.decode
> +++ b/target/arm/tcg/a64.decode
> @@ -960,6 +960,8 @@ USMMLA 0100 1110 100 ..... 10101 1 ..... .....
> @rrr_q1e0
> FCADD_90 0.10 1110 ..0 ..... 11100 1 ..... ..... @qrrr_e
> FCADD_270 0.10 1110 ..0 ..... 11110 1 ..... ..... @qrrr_e
>
> +FCMLA_v 0 q:1 10 1110 esz:2 0 rm:5 110 rot:2 1 rn:5 rd:5
> +
> ### Advanced SIMD scalar x indexed element
>
> FMUL_si 0101 1111 00 .. .... 1001 . 0 ..... ..... @rrx_h
> @@ -1041,6 +1043,9 @@ USDOT_vi 0.00 1111 10 .. .... 1111 . 0 .....
> ..... @qrrx_s
> BFDOT_vi 0.00 1111 01 .. .... 1111 . 0 ..... ..... @qrrx_s
> BFMLAL_vi 0.00 1111 11 .. .... 1111 . 0 ..... ..... @qrrx_h
>
> +FCMLA_vi 0 q:1 10 1111 10 . rm:5 0 rot:2 1 . 0 rn:5 rd:5 esz=1 idx=%hl
> +FCMLA_vi 0 q:1 10 1111 01 0 rm:5 0 rot:2 1 idx:1 0 rn:5 rd:5 esz=2
This doesn't look right. Bits [23:22] are the size field, but
here you have 10 and esz=1, and 01 and esz=2. I think the 01
and 10 should be the other way around.
In the size 10 case (MO_32), we should UNDEF for L=1 or Q=0,
which is to say that L must be 0 and Q must be 1. We hard-wire
the L bit (bit 21) to 0 in the decode, but we leave q:1 as it
is; I think it would be better for the second line here to have
a forced 1 in the q bit and set q=1 at the end of the line.
We also don't have the check for H==1 && Q==0 for the size 01 case.
I think we can do that in the decode file by splitting the 01
case into two lines, one for Q=0 and one for Q=1.
> +static bool trans_FCMLA_vi(DisasContext *s, arg_FCMLA_vi *a)
> +{
> + gen_helper_gvec_4_ptr *fn;
> +
> + if (!dc_isar_feature(aa64_fcma, s)) {
> + return false;
> + }
> + switch (a->esz) {
> + case MO_16:
> + if (!dc_isar_feature(aa64_fp16, s)) {
> + return false;
> + }
> + fn = gen_helper_gvec_fcmlah_idx;
> + break;
> + case MO_32:
> + if (!a->q && a->idx) {
> + return false;
> + }
I suspect this was supposed to be the size 01 H==1 && Q==0
check, but it's in the wrong case.
So my suggested fixup for this patch is:
diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
index 4b2a6ba302d..223eac3cac2 100644
--- a/target/arm/tcg/a64.decode
+++ b/target/arm/tcg/a64.decode
@@ -1043,8 +1043,9 @@ USDOT_vi 0.00 1111 10 .. .... 1111 . 0
..... ..... @qrrx_s
BFDOT_vi 0.00 1111 01 .. .... 1111 . 0 ..... ..... @qrrx_s
BFMLAL_vi 0.00 1111 11 .. .... 1111 . 0 ..... ..... @qrrx_h
-FCMLA_vi 0 q:1 10 1111 10 . rm:5 0 rot:2 1 . 0 rn:5 rd:5 esz=1 idx=%hl
-FCMLA_vi 0 q:1 10 1111 01 0 rm:5 0 rot:2 1 idx:1 0 rn:5 rd:5 esz=2
+FCMLA_vi 0 0 10 1111 01 idx:1 rm:5 0 rot:2 1 0 0 rn:5 rd:5 esz=1 q=0
+FCMLA_vi 0 1 10 1111 01 . rm:5 0 rot:2 1 . 0 rn:5 rd:5 esz=1 idx=%hl q=1
+FCMLA_vi 0 1 10 1111 10 0 rm:5 0 rot:2 1 idx:1 0 rn:5 rd:5 esz=2 q=1
# Floating-point conditional select
diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
index 0a54a9ef8f7..161fa2659c4 100644
--- a/target/arm/tcg/translate-a64.c
+++ b/target/arm/tcg/translate-a64.c
@@ -6033,9 +6033,6 @@ static bool trans_FCMLA_vi(DisasContext *s,
arg_FCMLA_vi *a)
fn = gen_helper_gvec_fcmlah_idx;
break;
case MO_32:
- if (!a->q && a->idx) {
- return false;
- }
fn = gen_helper_gvec_fcmlas_idx;
break;
default:
thanks
-- PMM
