Re: [PATCH v1 4/4] aarch64: testsuite: share test utils macros and use them

[Matthieu Longo <matthieu.longo@arm.com>]

Just to make sure I understood you well, your main concern is about the 
difficulty of verifying the test cases caused by the proposed changes.

Those difficulties can be summarized by the two following points:
1. Using macros which hide the instructions is not to be encouraged if 
there's little or no reduction in the size of the assembly file.
2. The macro adds lots of macro invocation messages to the error check 
files.

(1) The file you took as an example 
(gas/testsuite/gas/aarch64/sysreg/armv8_9-a-sysregs.s) by the way is one 
of the files that I didn't change to remove r=1 w=1 which are the 
default values, and also that are still using a different register (x3) 
from the default (x0). That's why it looks so verbose, and sometimes 
worse than the original.

In the proposed patch, the expansion of the helper macros always 
generates the same sequence of instructions: msr[r] and mr[r]s, and 
always in the same order.
I agree that the usage of macros might add some complexity in others 
cases outside of the system registers tests. However, here it makes 
things better in my experience.

The issues that I was trying to address here were:
- Making things more explicit: w=1, r=0 is clear, the instruction name 
is more cryptic than rw_sys_reg, and I often need to check the doc. Even 
after several times, I am still confused which one is the read 
instruction, and which one is the write one.
- Enforcing the order of emitted instructions (write before read) so 
that the reading of the generated assembly is easier (=more mechanical).
- Uniformizing the macros across the different files. People have been 
reimplementing the same macros again and again, each time in a slightly 
different way. I am not aware of the reason behind their choice, but 
from the naming, I would guess that readability was their goal.

Please, note that I didn't change every file, but only set up the 
boilerplates with some examples so that people starts using those macros.

If you have a look at others assembly files that were changed, do you 
still feel that the macros are not helping for readability ?
Your concern is fair. I am interested in feedback of others reviewers.

(2) Regarding the macro invocation messages when checking for errors, I 
agree that they are very bothering. I actually prepared a patch to a new 
command line option --no-info (similar to --no-warn) which allows to 
disable such annoying messages. I plan to publish it later, but I expect 
it to be a bit controversial as it adds a new command line options to 
the CLI interface. Consequently, I preferred not to make my changes 
dependent on it.

On 2024-02-27 16:31, Andrew Carlotti wrote:
> On Tue, Feb 27, 2024 at 10:59:17AM +0000, Matthieu Longo wrote:
>>
>> This patch rewrites assembly tests to use utils macros declared in
>> sysreg-test-utils.inc. Some tests were adapted to use the new macro
>> rw_sys_reg.
>> ---
>>   .../aarch64/sysreg/armv8_9-a-sysregs-bad.d    |   2 +-
>>   .../aarch64/sysreg/armv8_9-a-sysregs-bad.l    |  90 ++++-
>>   .../gas/aarch64/sysreg/armv8_9-a-sysregs.d    |   3 +-
>>   .../gas/aarch64/sysreg/armv8_9-a-sysregs.s    | 139 +++----
>>   .../gas/aarch64/sysreg/illegal-sysreg-3.d     |   2 +-
>>   .../gas/aarch64/sysreg/illegal-sysreg-4.d     |   2 +-
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-1.d   |   2 +
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-1.s   | 223 ++++++------
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-2.d   |   3 +-
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-2.s   |  47 ++-
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-3.d   |   3 +-
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-3.s   |  25 +-
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-6.d   |   2 +
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-6.s   |   7 +-
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-7.d   |   2 +
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-7.s   |  32 +-
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-8.d   |   2 +
>>   gas/testsuite/gas/aarch64/sysreg/sysreg-8.s   | 339 +++++++++---------
>>   .../gas/aarch64/sysreg/sysreg-test-utils.inc  |  32 ++
>>   gas/testsuite/gas/aarch64/sysreg/sysreg.d     |   6 +-
>>   gas/testsuite/gas/aarch64/sysreg/sysreg.s     |  63 ++--
>>   gas/testsuite/gas/aarch64/sysreg/sysreg128.d  |  42 +--
>>   gas/testsuite/gas/aarch64/sysreg/sysreg128.s  |  27 +-
>>   23 files changed, 576 insertions(+), 519 deletions(-)
>>   create mode 100644 gas/testsuite/gas/aarch64/sysreg/sysreg-test-utils.inc
>>
> ...
>> diff --git a/gas/testsuite/gas/aarch64/sysreg/armv8_9-a-sysregs.s b/gas/testsuite/gas/aarch64/sysreg/armv8_9-a-sysregs.s
>> index bf9019c9ac8..318d8bb9097 100644
>> --- a/gas/testsuite/gas/aarch64/sysreg/armv8_9-a-sysregs.s
>> +++ b/gas/testsuite/gas/aarch64/sysreg/armv8_9-a-sysregs.s
>> @@ -1,32 +1,23 @@
>> -	msr PMSDSFR_EL1, x3
>> -	mrs x3, PMSDSFR_EL1
>> +	.include "sysreg-test-utils.inc"
>> +
>> +.text
>> +	rw_sys_reg sys_reg=PMSDSFR_EL1 xreg=x3 r=1 w=1
>>   
>>   	mrs x0, ERXGSR_EL1
>>   
>> -	msr SCTLR2_EL1, x3
>> -	mrs x3, SCTLR2_EL1
>> -	msr SCTLR2_EL12, x3
>> -	mrs x3, SCTLR2_EL12
>> -	msr SCTLR2_EL2, x3
>> -	mrs x3, SCTLR2_EL2
>> -	msr SCTLR2_EL3, x3
>> -	mrs x3, SCTLR2_EL3
>> -
>> -	msr HDFGRTR2_EL2, x3
>> -	mrs x3, HDFGRTR2_EL2
>> -	msr HDFGWTR2_EL2, x3
>> -	mrs x3, HDFGWTR2_EL2
>> -	msr HFGRTR2_EL2, x3
>> -	mrs x3, HFGRTR2_EL2
>> -	msr HFGWTR2_EL2, x3
>> -	mrs x3, HFGWTR2_EL2
>> -
>> -	msr PFAR_EL1, x0
>> -	mrs x0, PFAR_EL1
>> -	msr PFAR_EL2, x0
>> -	mrs x0, PFAR_EL2
>> -	msr PFAR_EL12, x0
>> -	mrs x0, PFAR_EL12
>> +	rw_sys_reg sys_reg=SCTLR2_EL1 xreg=x3 r=1 w=1
>> +	rw_sys_reg sys_reg=SCTLR2_EL12 xreg=x3 r=1 w=1
>> +	rw_sys_reg sys_reg=SCTLR2_EL2 xreg=x3 r=1 w=1
>> +	rw_sys_reg sys_reg=SCTLR2_EL3 xreg=x3 r=1 w=1
>> +
>> +	rw_sys_reg sys_reg=HDFGRTR2_EL2 xreg=x3 r=1 w=1
>> +	rw_sys_reg sys_reg=HDFGWTR2_EL2 xreg=x3 r=1 w=1
>> +	rw_sys_reg sys_reg=HFGRTR2_EL2 xreg=x3 r=1 w=1
>> +	rw_sys_reg sys_reg=HFGWTR2_EL2 xreg=x3 r=1 w=1
>> +
>> +	rw_sys_reg sys_reg=PFAR_EL1 xreg=x0 r=1 w=1
>> +	rw_sys_reg sys_reg=PFAR_EL2 xreg=x0 r=1 w=1
>> +	rw_sys_reg sys_reg=PFAR_EL12 xreg=x0 r=1 w=1
> 
> This may be just my preference, and I know you're just extending existing
> practice here, but I'm not keen on hiding the actual instructions away behind
> macros (especially if there's little or no reduction in the size of the
> assembly file).  I think it makes it harder to check that the input assembly
> matches the output assembly, and I've seen several cases recently where such
> discrepancies were missed.  It also adds lots of macro invocation messages to
> the error check files.
> 
> In the case of a large multidimensional cross-product of operand values it
> might make more sense to use macros, but in that case you'd still need to check
> and include the full assembly contents in the .d file.   You can probably get
> equally effective test coverage in most cases by just checking each dimension
> separately anyway.