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

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

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.