Re: [PATCH v1 10/11] [Binutils] aarch64: Fix FEAT_B16B16 sve2 instruction constraints.

["Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>]

On 14/06/2024 16:44, Andre Vieira (lists) wrote:
> 
> 
> On 13/06/2024 16:44, Richard Earnshaw (lists) wrote:
>> On 12/06/2024 16:59, srinath wrote:
>>>
>>> Hi,
>>>
>>> This patch adds missing contraints to FEAT_B16B16 sve2 instructions
>>> bfclamp, bfmla and bfmls and add negative tests for all the bfloat
>>> instructions.
>>>
>>> Regression tested for aarch64-none-elf target and found no regressions.
>>>
>>> Ok for binutils-master?
>>>
>>> Regards,
>>> Srinath.
>>> ---
>>>   gas/testsuite/gas/aarch64/bfloat16-1.d        |   6 +
>>>   gas/testsuite/gas/aarch64/bfloat16-1.s        |   7 +-
>>>   .../gas/aarch64/bfloat16-2-invalid.d          |   4 +
>>>   .../gas/aarch64/bfloat16-2-invalid.l          | 265 ++++++++++++++++++
>>>   .../gas/aarch64/bfloat16-2-invalid.s          | 147 ++++++++++
>>>   gas/testsuite/gas/aarch64/bfloat16-bad.l      |   3 +
>>>   gas/testsuite/gas/aarch64/bfloat16-invalid.d  |   2 +-
>>>   gas/testsuite/gas/aarch64/bfloat16-invalid.l  |  17 +-
>>>   gas/testsuite/gas/aarch64/bfloat16-invalid.s  |   9 +-
>>>   opcodes/aarch64-tbl.h                         |  46 +--
>>>   10 files changed, 468 insertions(+), 38 deletions(-)
>>>   create mode 100644 gas/testsuite/gas/aarch64/bfloat16-2-invalid.d
>>>   create mode 100644 gas/testsuite/gas/aarch64/bfloat16-2-invalid.l
>>>   create mode 100644 gas/testsuite/gas/aarch64/bfloat16-2-invalid.s
>>>
>> --- a/gas/testsuite/gas/aarch64/bfloat16-invalid.l
>> +++ b/gas/testsuite/gas/aarch64/bfloat16-invalid.l
>> ...
>> +.*: Error: operand 3 must be the same register as operand 1 -- `bfmul .*
>> +.*: Error: operand 3 must be the same register as operand 1 -- `bfsub .*
>> +.*: Error: selected processor does not support `bfclamp z3.h,z4.h,z16.h'
>> +.*: Error: selected processor does not support `bfmla z3.h,z16.h,z6.h\[7\]'
>> +.*: Error: selected processor does not support `bfmls z3.h,z16.h,z6.h\[7\]'
>>
>> It doesn't seem right to me that a test that is supposedly checking for invalid operand combinations would be reporting unsupported instructions.  Perhaps these should be in a separate test with different command-line options?
> 
> I think you misunderstood here Richard, Srinath turned this test into a 'wrong command-line option/unsupported test' see:
> diff --git a/gas/testsuite/gas/aarch64/bfloat16-invalid.d b/gas/testsuite/gas/aarch64/bfloat16-invalid.d
> index 8f24dc62083..02e3e8d8e3d 100644
> --- a/gas/testsuite/gas/aarch64/bfloat16-invalid.d
> +++ b/gas/testsuite/gas/aarch64/bfloat16-invalid.d
> @@ -1,4 +1,4 @@
> -#name: Test Bfloat16 instructions with wrong operand combinations
> +#name: Negative test with missing +b16b16 bfloat16 flag.

That doesn't make sense to me.  Why change an existing test for one thing into a different test that does something else?  The existing test entries in the file now no-longer match the test summary.

This sort of change should be highlighted in the patch summary; patch review for complex patches is difficult enough as it is, without explanations as to why certain changes are made it's nearly impossible to intuit the reasoning behind the changes.

> 
> The reason you got confused I think is because it also has wrong operand errors, which is because gas reports these before it reports missing support. It does so for other features too mind you, so we are consistent here AFAICT.

Everything in the existing bfloat16-invalid.l is an operand test, so why conflate this with something else?  That doesn't look consistent to me.

Perhaps this could be addressed by writing it in the style of mops_invalid.[sdl] where we change the architecture features on the fly to test different errors.  It would also help if the assembler source had some comments about the expected failures.  

> 
> I do think however, that this test should have at least one of each of the new instructions with proper operands, so that we can make sure the test checks these are unsupported. For instance bfadd is only in here with wrong operands. So it's not actually testing that we don't support bfadd with -march=armv9.4-a
> 
> Whether we should also include wrong operands here, testing that gas checks those before support for the actual instruction is a decision that I'll leave to you Richard.  On one side if we decide to change the behavior we'd have to go back and change these files, but that itself may be a benefit to having the tests... we could make sure the behavior doesn't change unnoticed.


R.