This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [PATCH][BINUTILS][AARCH64] Fix error checking for SIMD udot (by element)
- From: Tamar Christina <Tamar dot Christina at arm dot com>
- To: Matthew Malcomson <Matthew dot Malcomson at arm dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Cc: Richard Earnshaw <Richard dot Earnshaw at arm dot com>, nd <nd at arm dot com>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>
- Date: Thu, 4 Oct 2018 11:56:40 +0000
- Subject: RE: [PATCH][BINUTILS][AARCH64] Fix error checking for SIMD udot (by element)
- References: <VI1PR0801MB2014E1720C036970BF196097E0EA0@VI1PR0801MB2014.eurprd08.prod.outlook.com>
Hi Matthew,
Thanks for fixing this!
I'm not a maintainer so you still need approval, but this looks good to me.
We should also backport this to 2.31
Thanks,
Tamar
> -----Original Message-----
> From: binutils-owner@sourceware.org <binutils-owner@sourceware.org>
> On Behalf Of Matthew Malcomson
> Sent: Thursday, October 4, 2018 10:16
> To: binutils@sourceware.org
> Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; nd <nd@arm.com>;
> Marcus Shawcroft <Marcus.Shawcroft@arm.com>
> Subject: [PATCH][BINUTILS][AARCH64] Fix error checking for SIMD udot (by
> element)
>
> [PATCH][BINUTILS][AARCH64] Fix error checking for SIMD udot (by element)
>
> The SIMD UDOT instruction assembly has an unusual operand that selects a
> single
> 32 bit element with the mnemonic 4B.
> This unusual mnemonic is handled by a special operand qualifier and
> associated qualifier data in `aarch64_opnd_qualifiers`.
>
> The current qualifier data describes 4 1-byte elements with the structure {1,
> 4, 0x0, "4b", OQK_OPD_VARIANT} This makes sense, as the instruction does
> work on 4 1-byte elements, however some logic in the
> `operand_general_constraint_met_p` makes assumptions about the range
> of index allowed when selecting a SIMD_ELEMENT depending on element
> size.
> That function reasons that e.g. in order to select a byte-sized element in a 16
> byte V register an index must allow selection of one of the 16 elements and
> hence its range will be in [0,15].
>
> This reasoning breaks with the above description of a 4 part selection of 1
> byte elements and allows an index outside the valid [0,3] range, triggering an
> assert later on in the program in `aarch64_ins_reglane`.
>
> vshcmd: > echo 'udot v0.2s, v1.8b, v2.4b[4]' | ../src/binutils-build/gas/as-new
> -march=armv8.4-a
> as-new: ../../binutils-gdb/opcodes/aarch64-asm.c:134: aarch64_ins_reglane:
> Assertion `reglane_index < 4' failed.
> {standard input}: Assembler messages:
> {standard input}:1: Internal error (Aborted).
> Please report this bug.
>
>
> This patch changes the operand qualifier data so that it describes a single
> 32 bit element.
> {4, 1, 0x0, "4b", OQK_OPD_VARIANT}
> Hence the calculation in `operand_general_constraint_met_p` provides the
> correct answer and the usual error checking machinery is used.
>
> vshcmd: > echo 'udot v0.2s, v1.8b, v2.4b[4]' | ../src/binutils-build/gas/as-new
> -march=armv8.4-a {standard input}: Assembler messages:
> {standard input}:1: Error: register element index out of range 0 to 3 at
> operand 3 -- `udot v0.2s,v1.8b,v2.4b[4]'
>
> Ran tests on aarch64-none-linux-gnu.
> Ok for trunk?
>
> gas/ChangeLog:
>
> 2018-10-04 Matthew Malcomson <matthew.malcomson@arm.com>
>
> * testsuite/gas/aarch64/illegal-dotproduct.d: New test.
> * testsuite/gas/aarch64/illegal-dotproduct.l: New test.
> * testsuite/gas/aarch64/illegal-dotproduct.s: New test.
>
> opcodes/ChangeLog:
>
> 2018-10-04 Matthew Malcomson <matthew.malcomson@arm.com>
>
> * aarch64-opc.c (struct operand_qualifier_data): Change qualifier
> data
> corresponding to AARCH64_OPND_QLF_S_4B qualifier.
>
>
> ############### Attachment also inlined for ease of reply
> ###############
>
>
> diff --git a/gas/testsuite/gas/aarch64/illegal-dotproduct.d
> b/gas/testsuite/gas/aarch64/illegal-dotproduct.d
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..3f8928da83bb64891528cf5860
> 236f4b73c184b8
> --- /dev/null
> +++ b/gas/testsuite/gas/aarch64/illegal-dotproduct.d
> @@ -0,0 +1,4 @@
> +#as: -march=armv8.2-a+dotprod
> +#name: Invalid dotproduct instructions.
> +#source: illegal-dotproduct.s
> +#error_output: illegal-dotproduct.l
> diff --git a/gas/testsuite/gas/aarch64/illegal-dotproduct.l
> b/gas/testsuite/gas/aarch64/illegal-dotproduct.l
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..06d0d78b8dc91d7d140415aa5
> a4a67d4b20edefd
> --- /dev/null
> +++ b/gas/testsuite/gas/aarch64/illegal-dotproduct.l
> @@ -0,0 +1,13 @@
> +[^:]+: Assembler messages:
> +[^:]+:[0-9]+: Error: register element index out of range 0 to 3 at operand 3 --
> `udot V0.2S,V0.8B,V0.4B\[4\]'
> +[^:]+:[0-9]+: Error: operand mismatch -- `udot V0.4S,V0.8B,V0.4B\[4\]'
> +[^:]+:[0-9]+: Info: did you mean this\?
> +[^:]+:[0-9]+: Info: udot v0.2s, v0.8b, v0.4b\[4\]
> +[^:]+:[0-9]+: Info: other valid variant\(s\):
> +[^:]+:[0-9]+: Info: udot v0.4s, v0.16b, v0.4b\[4\]
> +[^:]+:[0-9]+: Error: register element index out of range 0 to 3 at operand 3 --
> `sdot V0.2S,V0.8B,V0.4B\[4\]'
> +[^:]+:[0-9]+: Error: operand mismatch -- `sdot V0.2S,V0.8B,V0.4H\[4\]'
> +[^:]+:[0-9]+: Info: did you mean this\?
> +[^:]+:[0-9]+: Info: sdot v0.2s, v0.8b, v0.4b\[4\]
> +[^:]+:[0-9]+: Info: other valid variant\(s\):
> +[^:]+:[0-9]+: Info: sdot v0.4s, v0.16b, v0.4b\[4\]
> diff --git a/gas/testsuite/gas/aarch64/illegal-dotproduct.s
> b/gas/testsuite/gas/aarch64/illegal-dotproduct.s
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..9c714ae54d8e0b45dea8d7cb6
> 455801d53a00ddc
> --- /dev/null
> +++ b/gas/testsuite/gas/aarch64/illegal-dotproduct.s
> @@ -0,0 +1,4 @@
> +UDOT V0.2S, V0.8B, V0.4B[4]
> +UDOT V0.4S, V0.8B, V0.4B[4]
> +SDOT V0.2S, V0.8B, V0.4B[4]
> +SDOT V0.2S, V0.8B, V0.4H[4]
> diff --git a/opcodes/aarch64-opc.c b/opcodes/aarch64-opc.c index
> ba2af7bfc26d2f760f19cdbdd07ebe5535308d72..e59240c98db3ea8472b992a42
> 3b53db6340033ea 100644
> --- a/opcodes/aarch64-opc.c
> +++ b/opcodes/aarch64-opc.c
> @@ -698,7 +698,7 @@ struct operand_qualifier_data
> aarch64_opnd_qualifiers[] =
> {4, 1, 0x2, "s", OQK_OPD_VARIANT},
> {8, 1, 0x3, "d", OQK_OPD_VARIANT},
> {16, 1, 0x4, "q", OQK_OPD_VARIANT},
> - {1, 4, 0x0, "4b", OQK_OPD_VARIANT},
> + {4, 1, 0x0, "4b", OQK_OPD_VARIANT},
>
> {1, 4, 0x0, "4b", OQK_OPD_VARIANT},
> {1, 8, 0x0, "8b", OQK_OPD_VARIANT},
> @@ -2501,6 +2501,7 @@ operand_general_constraint_met_p (const
> aarch64_opnd_info *opnds, int idx,
> else
> num = 16;
> num = num / aarch64_get_qualifier_esize (qualifier) - 1;
> + assert (aarch64_get_qualifier_nelem (qualifier) == 1);
>
> /* Index out-of-range. */
> if (!value_in_range_p (opnd->reglane.index, 0, num))