We were extracting the M register twice, once incorrectly
as M:vm and once correctly as rm.  Remove the incorrect
name and remove the incorrect decode.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/neon-shared.decode |  4 +-
 target/arm/translate-neon.c   | 90 +++++++++++++++--------------------
 2 files changed, 40 insertions(+), 54 deletions(-)

diff --git a/target/arm/neon-shared.decode b/target/arm/neon-shared.decode
index ca0c699072..facb621450 100644
--- a/target/arm/neon-shared.decode
+++ b/target/arm/neon-shared.decode
@@ -61,8 +61,8 @@ VCMLA_scalar   1111 1110 0 . rot:2 .... .... 1000 . q:1 index:1 0 vm:4 \
 VCMLA_scalar   1111 1110 1 . rot:2 .... .... 1000 . q:1 . 0 .... \
                vm=%vm_dp vn=%vn_dp vd=%vd_dp size=2 index=0
 
-VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 rm:4 \
-               vm=%vm_dp vn=%vn_dp vd=%vd_dp
+VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 vm:4 \
+               vn=%vn_dp vd=%vd_dp
 
 %vfml_scalar_q0_rm 0:3 5:1
 %vfml_scalar_q1_index 5:1 3:1
diff --git a/target/arm/translate-neon.c b/target/arm/translate-neon.c
index a0e267694b..52b75ff76f 100644
--- a/target/arm/translate-neon.c
+++ b/target/arm/translate-neon.c
@@ -151,6 +151,36 @@ static void neon_store_element64(int reg, int ele, MemOp size, TCGv_i64 var)
     }
 }
 
+static bool do_neon_ddda(DisasContext *s, int q, int vd, int vn, int vm,
+                         int data, gen_helper_gvec_4 *fn_gvec)
+{
+    /* UNDEF accesses to D16-D31 if they don't exist. */
+    if (((vd | vn | vm) & 0x10) && !dc_isar_feature(aa32_simd_r32, s)) {
+        return false;
+    }
+
+    /*
+     * UNDEF accesses to odd registers for each bit of Q.
+     * Q will be 0b111 for all Q-reg instructions, otherwise
+     * when we have mixed Q- and D-reg inputs.
+     */
+    if (((vd & 1) * 4 | (vn & 1) * 2 | (vm & 1)) & q) {
+        return false;
+    }
+
+    if (!vfp_access_check(s)) {
+        return true;
+    }
+
+    int opr_sz = q ? 16 : 8;
+    tcg_gen_gvec_4_ool(vfp_reg_offset(1, vd),
+                       vfp_reg_offset(1, vn),
+                       vfp_reg_offset(1, vm),
+                       vfp_reg_offset(1, vd),
+                       opr_sz, opr_sz, data, fn_gvec);
+    return true;
+}
+
 static bool do_neon_ddda_fpst(DisasContext *s, int q, int vd, int vn, int vm,
                               int data, ARMFPStatusFlavour fp_flavor,
                               gen_helper_gvec_4_ptr *fn_gvec_ptr)
@@ -241,35 +271,13 @@ static bool trans_VCADD(DisasContext *s, arg_VCADD *a)
 
 static bool trans_VDOT(DisasContext *s, arg_VDOT *a)
 {
-    int opr_sz;
-    gen_helper_gvec_4 *fn_gvec;
-
     if (!dc_isar_feature(aa32_dp, s)) {
         return false;
     }
-
-    /* UNDEF accesses to D16-D31 if they don't exist. */
-    if (!dc_isar_feature(aa32_simd_r32, s) &&
-        ((a->vd | a->vn | a->vm) & 0x10)) {
-        return false;
-    }
-
-    if ((a->vn | a->vm | a->vd) & a->q) {
-        return false;
-    }
-
-    if (!vfp_access_check(s)) {
-        return true;
-    }
-
-    opr_sz = (1 + a->q) * 8;
-    fn_gvec = a->u ? gen_helper_gvec_udot_b : gen_helper_gvec_sdot_b;
-    tcg_gen_gvec_4_ool(vfp_reg_offset(1, a->vd),
-                       vfp_reg_offset(1, a->vn),
-                       vfp_reg_offset(1, a->vm),
-                       vfp_reg_offset(1, a->vd),
-                       opr_sz, opr_sz, 0, fn_gvec);
-    return true;
+    return do_neon_ddda(s, a->q * 7, a->vd, a->vn, a->vm, 0,
+                        a->u
+                        ? gen_helper_gvec_udot_b
+                        : gen_helper_gvec_sdot_b);
 }
 
 static bool trans_VFML(DisasContext *s, arg_VFML *a)
@@ -323,35 +331,13 @@ static bool trans_VCMLA_scalar(DisasContext *s, arg_VCMLA_scalar *a)
 
 static bool trans_VDOT_scalar(DisasContext *s, arg_VDOT_scalar *a)
 {
-    gen_helper_gvec_4 *fn_gvec;
-    int opr_sz;
-
     if (!dc_isar_feature(aa32_dp, s)) {
         return false;
     }
-
-    /* UNDEF accesses to D16-D31 if they don't exist. */
-    if (!dc_isar_feature(aa32_simd_r32, s) &&
-        ((a->vd | a->vn) & 0x10)) {
-        return false;
-    }
-
-    if ((a->vd | a->vn) & a->q) {
-        return false;
-    }
-
-    if (!vfp_access_check(s)) {
-        return true;
-    }
-
-    fn_gvec = a->u ? gen_helper_gvec_udot_idx_b : gen_helper_gvec_sdot_idx_b;
-    opr_sz = (1 + a->q) * 8;
-    tcg_gen_gvec_4_ool(vfp_reg_offset(1, a->vd),
-                       vfp_reg_offset(1, a->vn),
-                       vfp_reg_offset(1, a->rm),
-                       vfp_reg_offset(1, a->vd),
-                       opr_sz, opr_sz, a->index, fn_gvec);
-    return true;
+    return do_neon_ddda(s, a->q * 6, a->vd, a->vn, a->vm, a->index,
+                        a->u
+                        ? gen_helper_gvec_udot_idx_b
+                        : gen_helper_gvec_sdot_idx_b);
 }
 
 static bool trans_VFML_scalar(DisasContext *s, arg_VFML_scalar *a)
-- 
2.25.1

On Fri, 30 Apr 2021 at 22:23, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We were extracting the M register twice, once incorrectly
> as M:vm and once correctly as rm.  Remove the incorrect
> name and remove the incorrect decode.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/neon-shared.decode |  4 +-
>  target/arm/translate-neon.c   | 90 +++++++++++++++--------------------
>  2 files changed, 40 insertions(+), 54 deletions(-)
>
> diff --git a/target/arm/neon-shared.decode b/target/arm/neon-shared.decode
> index ca0c699072..facb621450 100644
> --- a/target/arm/neon-shared.decode
> +++ b/target/arm/neon-shared.decode
> @@ -61,8 +61,8 @@ VCMLA_scalar   1111 1110 0 . rot:2 .... .... 1000 . q:1 index:1 0 vm:4 \
>  VCMLA_scalar   1111 1110 1 . rot:2 .... .... 1000 . q:1 . 0 .... \
>                 vm=%vm_dp vn=%vn_dp vd=%vd_dp size=2 index=0
>
> -VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 rm:4 \
> -               vm=%vm_dp vn=%vn_dp vd=%vd_dp
> +VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 vm:4 \
> +               vn=%vn_dp vd=%vd_dp

Is it possible to make this kind of bug a decodetree error?
It seems unlikely that there's a use for having a bit which is
decoded both by a %foo field specification and also in some
other way...


>
>  %vfml_scalar_q0_rm 0:3 5:1
>  %vfml_scalar_q1_index 5:1 3:1
> diff --git a/target/arm/translate-neon.c b/target/arm/translate-neon.c
> index a0e267694b..52b75ff76f 100644
> --- a/target/arm/translate-neon.c
> +++ b/target/arm/translate-neon.c
> @@ -151,6 +151,36 @@ static void neon_store_element64(int reg, int ele, MemOp size, TCGv_i64 var)
>      }
>  }
>
> +static bool do_neon_ddda(DisasContext *s, int q, int vd, int vn, int vm,
> +                         int data, gen_helper_gvec_4 *fn_gvec)

This patch seems to be doing more than its commit message suggests.
If we want to share code between trans_VDOT and trans_VDOT_scalar
can we do that refactoring in its own patch, please ?

thanks
-- PMM

On 5/13/21 2:25 PM, Peter Maydell wrote:
>> -VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 rm:4 \
>> -               vm=%vm_dp vn=%vn_dp vd=%vd_dp
>> +VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 vm:4 \
>> +               vn=%vn_dp vd=%vd_dp
> 
> Is it possible to make this kind of bug a decodetree error?
> It seems unlikely that there's a use for having a bit which is
> decoded both by a %foo field specification and also in some
> other way...

That's not what's happening here.  This has separate fields "rm" and "vm" 
decoded in different ways.


r~

On Sat, 15 May 2021 at 18:13, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/13/21 2:25 PM, Peter Maydell wrote:
> >> -VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 rm:4 \
> >> -               vm=%vm_dp vn=%vn_dp vd=%vd_dp
> >> +VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 vm:4 \
> >> +               vn=%vn_dp vd=%vd_dp
> >
> > Is it possible to make this kind of bug a decodetree error?
> > It seems unlikely that there's a use for having a bit which is
> > decoded both by a %foo field specification and also in some
> > other way...
>
> That's not what's happening here.  This has separate fields "rm" and "vm"
> decoded in different ways.

But they overlap: rm:4 in the pattern itself is using bits [3:0],
and "vm=%vm_dp" is also using [3:0] because the %vm_dp field
specifier is defined as "5:1 0:4". I'm suggesting that if the
pattern uses a field specifier we should check that none of the
bits in that field specifier are used in the pattern for some
other purpose (here 'u' and 'rm').

thanks
-- PMM

On 5/16/21 11:09 AM, Peter Maydell wrote:
> On Sat, 15 May 2021 at 18:13, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 5/13/21 2:25 PM, Peter Maydell wrote:
>>>> -VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 rm:4 \
>>>> -               vm=%vm_dp vn=%vn_dp vd=%vd_dp
>>>> +VDOT_scalar    1111 1110 0 . 10 .... .... 1101 . q:1 index:1 u:1 vm:4 \
>>>> +               vn=%vn_dp vd=%vd_dp
>>>
>>> Is it possible to make this kind of bug a decodetree error?
>>> It seems unlikely that there's a use for having a bit which is
>>> decoded both by a %foo field specification and also in some
>>> other way...
>>
>> That's not what's happening here.  This has separate fields "rm" and "vm"
>> decoded in different ways.
> 
> But they overlap: rm:4 in the pattern itself is using bits [3:0],
> and "vm=%vm_dp" is also using [3:0] because the %vm_dp field
> specifier is defined as "5:1 0:4". I'm suggesting that if the
> pattern uses a field specifier we should check that none of the
> bits in that field specifier are used in the pattern for some
> other purpose (here 'u' and 'rm').

We do this, more or less, for sve:

# Three register operand, with governing predicate, vector element size
@rda_pg_rn_rm   ........ esz:2 . rm:5  ... pg:3 rn:5 rd:5 \
                 &rprrr_esz ra=%reg_movprfx

where ra and rd overlap.  Though ra and rd overlap exactly, so perhaps that's 
not quite the same as vm above, overlapping both rm and index.


r~

On 5/13/21 2:25 PM, Peter Maydell wrote:
>> +static bool do_neon_ddda(DisasContext *s, int q, int vd, int vn, int vm,
>> +                         int data, gen_helper_gvec_4 *fn_gvec)
> 
> This patch seems to be doing more than its commit message suggests.
> If we want to share code between trans_VDOT and trans_VDOT_scalar
> can we do that refactoring in its own patch, please ?

It appears as if a rebasing error squashed two patches together
(git commit --amend vs git rebase --continue after conflicts).

I nearly made the identical mistake while splitting this apart again just now.  :-P


r~