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

[Andrew Carlotti <andrew.carlotti@arm.com>]

On Fri, Mar 01, 2024 at 10:10:45AM +0000, Matthieu Longo wrote:
> 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.

My point is more about it being more difficult (in my opinion) to compare the
input and output when one of them uses macros, regardless of any increase or
decrease in codesize.

> (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.

Indeed - this is just my preference at the moment, and I don't think it should
prevent this patch being merged (particularly since you're just homogenizing
existing macro usage).  Plenty of other people have used the macros before you,
and must therefore have considered them to be beneficial.

> (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.