[PATCH][BINUTILS][AARCH64] Fix error checking for SIMD udot (by element)

Discussion:

Matthew Malcomson

2018-10-04 09:15:36 UTC

[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 <***@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 <***@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..3f8928da83bb64891528cf5860236f4b73c184b8
--- /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..06d0d78b8dc91d7d140415aa5a4a67d4b20edefd
--- /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..9c714ae54d8e0b45dea8d7cb6455801d53a00ddc
--- /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..e59240c98db3ea8472b992a423b53db6340033ea 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))

Tamar Christina

2018-10-04 11:56:40 UTC

Permalink

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-----
On Behalf Of Matthew Malcomson
Sent: Thursday, October 4, 2018 10:16
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
Assertion `reglane_index < 4' failed.
{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
{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?
* testsuite/gas/aarch64/illegal-dotproduct.d: New test.
* testsuite/gas/aarch64/illegal-dotproduct.l: New test.
* testsuite/gas/aarch64/illegal-dotproduct.s: New test.
* 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 @@
+[^:]+:[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: 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: 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))

Matthew Malcomson

2018-10-15 14:21:09 UTC

Permalink

ping

Post by Tamar Christina
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

Nick Clifton

2018-10-16 16:16:45 UTC

Permalink

Hi Matthew,

ping

Sorry...

    * testsuite/gas/aarch64/illegal-dotproduct.d: New test.
    * testsuite/gas/aarch64/illegal-dotproduct.l: New test.
    * testsuite/gas/aarch64/illegal-dotproduct.s: New test.
    * aarch64-opc.c (struct operand_qualifier_data): Change qualifier
data
    corresponding to AARCH64_OPND_QLF_S_4B qualifier.

Approved - please apply.

Cheers
Nick

Tamar Christina

2018-10-16 16:20:37 UTC

Permalink

Hi Nick,

Is this also ok for backporting to binutils-2.31?

-----Original Message-----
On Behalf Of Nick Clifton
Sent: Tuesday, October 16, 2018 17:17
Subject: Re: [PATCH][BINUTILS][AARCH64] Fix error checking for SIMD udot
(by element)
Hi Matthew,

ping

Sorry...

    * testsuite/gas/aarch64/illegal-dotproduct.d: New test.
    * testsuite/gas/aarch64/illegal-dotproduct.l: New test.
    * testsuite/gas/aarch64/illegal-dotproduct.s: New test.
    * aarch64-opc.c (struct operand_qualifier_data): Change
qualifier data
    corresponding to AARCH64_OPND_QLF_S_4B qualifier.

Approved - please apply.
Ch

Nick Clifton

2018-10-16 16:40:58 UTC

Permalink

Hi Tamar,

Post by Tamar Christina
Is this also ok for backporting to binutils-2.31?

Yes, please do.

Cheers
Nick