Re: [PATCH v3 15/16] [gdb/testsuite] sme: Add SVE/SME testcases

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello,

Luis Machado <luis.machado@arm.com> writes:

> Hi,
>
> On 8/4/23 01:59, Thiago Jung Bauermann wrote:
>> 
>> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>> diff --git a/gdb/testsuite/gdb.arch/aarch64-sme-core-0.exp
>>> b/gdb/testsuite/gdb.arch/aarch64-sme-core-0.exp
>>> new file mode 100644
>>> index 00000000000..c4755346bc8
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.arch/aarch64-sme-core-0.exp
>>> @@ -0,0 +1,18 @@
>>> +# Copyright 2023 Free Software Foundation, Inc.
>>> +
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 3 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>> +
>>> +set id_start 0
>>> +set id_end 24
>> 
>> It would be useful to have a comment mentioning that the range above
>> tests the fpsimd state.
>> 
>> Same comment for the other aarch64-sme-*.exp tests.
>> 
>
> Yeah. That can be a bit obscure. I'll add some more documentation to that effect.
>
> The current arrangement is mostly aiming at splitting these tests in a way that minimizes
> the total run time.

That is a nice idea.

> I'm considering further splitting these, putting each test with SVL == 256 into its own
> file. Those are the ones that take the longest to run. The others can be grouped.
>
> Right now the split is based on continuous ranges (0~4, 5~9 etc). I may change that to a
> list
> instead. That will allow us to clearly specify the tests we want for each sub-file,
> hopefully
> causing each sub-test to have more or less the same run time.

Sounds good. Thanks for the explanation.

>>> +source $srcdir/$subdir/aarch64-sme-core.exp.tcl
>> 
>> <snip>
>> 
>>> +#
>>> +# Return the state string based on STATE
>>> +#
>>> +proc state_id_to_state_string { state } {
>>> +  if {$state == 0} {
>>> +    return "fpsimd"
>>> +  } elseif {$state == 1} {
>>> +    return "sve"
>>> +  } elseif {$state == 2} {
>>> +    return "ssve"
>>> +  } elseif {$state == 3} {
>>> +    return "za"
>>> +  } elseif {$state == 4} {
>>> +    return "za_ssve"
>>> +  }
>>> +}
>>> +
>>> +#
>>> +# Given a test ID, return the string representing the register state.
>>> +# The state is one of fpsimd, sve, ssve, za and za_ssve.
>>> +#
>>> +proc test_id_to_state { id } {
>>> +  set state [expr $id / 25]
>>> +
>>> +  return [state_id_to_state_string $state]
>>> +}
>>> +
>>> +#
>>> +# Given a test ID, return the associated vector length.
>>> +#
>>> +proc test_id_to_vl { id } {
>>> +  return [expr 16 << (($id / 5) % 5)]
>>> +}
>>> +
>>> +#
>>> +# Given a test ID, return the associated streaming vector length.
>>> +#
>>> +proc test_id_to_svl { id } {
>>> +  return [expr 16 << ($id % 5)]
>>> +}
>> 
>> I suggest adding an sme_ prefix to the procedures above, since "id" here
>> is specific to SME testcases.
>> 
>
> See below.
>
>>> +#
>>> +# With register STATE, vector length VL and streaming vector length SVL,
>>> +# run some register state checks to make sure the values are the expected
>>> +# ones
>>> +#
>>> +proc check_state { state vl svl } {
>>> +    # The FPSIMD registers are initialized with a value of 0x55 (85)
>>> +    # for each byte.
>>> +    #
>>> +    # The SVE registers are initialized with a value of 0xff (255) for each
>>> +    # byte, including the predicate registers and FFR.
>>> +    #
>>> +    # The SME (ZA) register is initialized with a value of 0xaa (170) for
>>> +    # each byte.
>>> +
>>> +    # Check VG to make sure it is correct
>>> +    set expected_vg [expr $vl / 8]
>>> +    # If streaming mode is enabled, then vg is actually svg.
>>> +    if {$state == "ssve" || $state == "za_ssve"} {
>>> +	set expected_vg [expr $svl / 8]
>>> +    }
>>> +    gdb_test "print \$vg" " = ${expected_vg}"
>>> +
>>> +    # Check SVG to make sure it is correct
>>> +    set expected_svg [expr $svl / 8]
>>> +    gdb_test "print \$svg" " = ${expected_svg}"
>>> +
>>> +    # Check the value of SVCR.
>>> +    gdb_test "print \$svcr" [get_svcr_value $state]
>>> +
>>> +    # When we have any SVE or SSVE state, the FPSIMD registers will have
>>> +    # the same values as the SVE/SSVE Z registers.
>>> +    set fpsimd_byte 85
>>> +    if {$state == "sve" || $state == "ssve" || $state == "za_ssve"} {
>>> +	set fpsimd_byte 255
>>> +    }
>>> +
>>> +    set sve_byte 255
>>> +    if {$state == "fpsimd" || $state == "za"} {
>>> +	set sve_byte 85
>>> +    }
>>> +
>>> +    # Check FPSIMD registers
>>> +    check_fpsimd_regs $fpsimd_byte $state $vl $svl
>>> +    # Check SVE registers
>>> +    check_sve_regs $sve_byte $state $vl $svl
>>> +    # Check SME registers
>>> +    check_sme_regs 170 $state $svl
>>> +}
>> 
>> Also here, perhaps call this function "check_sme_state"?
>> 
>
> Maybe. Instead of renaming these functions, how about renaming the helper .exp file
> from aarch64.exp to aarch64-sme.exp? Or maybe split it into a sme-specific part and
> a generic part.

The 1d_array and 2d_array functions are even generic enough to go in gdb.exp.

> Moving forward, I'd like to make these tests cover SVE as well. In that case, we would be
> able to reuse some of these functions to raise the coverage of tests.
>
> What do you think?

I like the idea. Perhaps rename to aarch64-scalable.exp so that it
covers both SVE and SME? Also some functions that are currently in
gdb.exp could go there, such as allow_aarch64_sve_tests,
aarch64_initialize_sve_information, aarch64_supports_sve_vl and their
SME counterparts.

-- 
Thiago