Re: [PATCH v6 77/82] target/arm: Fix decode for VDOT (indexed)

[Peter Maydell]

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