Richard Earnshaw <richard.earns...@arm.com> writes:
> On 07/05/2025 17:28, Richard Earnshaw (lists) wrote:
>> On 07/05/2025 16:54, Richard Sandiford wrote:
>>> Richard Earnshaw <richard.earns...@arm.com> writes:
>>>> On 07/05/2025 13:57, Richard Sandiford wrote:
>>>>> Kyrylo Tkachov <ktkac...@nvidia.com> writes:
>>>>>>> On 7 May 2025, at 12:27, Karl Meakin <karl.mea...@arm.com> wrote:
>>>>>>>
>>>>>>> Commit the test file `cmpbr.c` before rules for generating the new
>>>>>>> instructions are added, so that the changes in codegen are more obvious
>>>>>>> in the next commit.
>>>>>>
>>>>>> I guess that’s an LLVM best practice.
>>>>>> In GCC since we have the check-function-bodies mechanism we usually
>>>>>> prefer to include the relevant test together with the patch that adds
>>>>>> the optimization.
>>>>>> But this is not wrong either.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>
>>>>>>> * gcc.target/aarch64/cmpbr.c: New test.
>>>>>>> ---
>>>>>>> gcc/testsuite/gcc.target/aarch64/cmpbr.c | 1378 ++++++++++++++++++++++
>>>>>>> 1 file changed, 1378 insertions(+)
>>>>>>> create mode 100644 gcc/testsuite/gcc.target/aarch64/cmpbr.c
>>>>>>>
>>>>>>> diff --git a/gcc/testsuite/gcc.target/aarch64/cmpbr.c
>>>>>>> b/gcc/testsuite/gcc.target/aarch64/cmpbr.c
>>>>>>> new file mode 100644
>>>>>>> index 00000000000..728d6ead91c
>>>>>>> --- /dev/null
>>>>>>> +++ b/gcc/testsuite/gcc.target/aarch64/cmpbr.c
>>>>>>> @@ -0,0 +1,1378 @@
>>>>>>> +/* Test that the instructions added by FEAT_CMPBR are emitted */
>>>>>>> +/* { dg-do compile } */
>>>>>>> +/* { dg-options "-march=armv9.5-a+cmpbr -O2" } */
>>>>>>> +/* { dg-final { check-function-bodies "**" "" "" } } */
>>>>>>
>>>>>> As you’ll be adding new instructions to the compiler it’d be good to
>>>>>> have it a dg-do assemble test where possible.
>>>>>
>>>>> Agreed FWIW, but:
>>>>>
>>>>>> For that you’ll need to create a new aarch64_asm_cmpbr_ok target and use
>>>>>> it like so to fallback to dg-do compile when the assembler is too old:
>>>>>> /* { dg-do compile { target aarch64_asm_cmpbr_ok } } */
>>>>>
>>>>> ...dg-do assemble for this one :)
>>>>
>>>> I don't think that works. If the first dg-do fails the test is just
>>>> skipped.
>>>>
>>>> You need to replicate the test with separate dg-do directives, IIRC.
>>>
>>> Hmm, can you remember the circumstances when you saw that?
>>> We've been using the construct that Kyrill suggested with apparent
>>> success in things like aarch64-sve2-acle-asm.exp. E.g.:
>>
>> Well, the implementation of dg-do contains the comment:
>>
>> # Note: A previous occurrence of `dg-do' with target/xfail selectors
>> # is a user mistake. We clobber previous values here.
>>
>> So one might interpret that as meaning multiple dg-do's are not intended to
>> be supported.
>>
>> But I might have misremembered the exact scenario I was facing. I think it
>> might have been that a test failed to fall back to the dg-do-default if a
>> specific dg-do didn't match. The scenario I remember was something like
>> dg-do-default = compile, then the test was trying to change that to execute
>> if HW was available; but that meant that if it wasn't we didn't fall back to
>> checking the assembler output.
>>
>
> The comment at the head of the function says:
>
> # Multiple instances are supported (since we don't support target and xfail
> # selectors on one line), though it doesn't make much sense to change the
> # compile/assemble/link/run field. Nor does it make any sense to have
> # multiple lines of target selectors (use one line).
>
> So maybe the code is intended to support multiple reasons for skipping the
> test (but why not use require-effective-target
> for that).
>
> I'm not sure now what's going on...
Richard and I discussed this more off-list, and it turns out that the
above construct started to work after:
https://git.savannah.gnu.org/gitweb/?p=dejagnu.git;a=commit;h=569f8718b534a2cd9511a7d640352eb0126ff492
which was first releasted in 1.6 (9 years ago). Before that, the "what"
(compile/assemble/etc.) in the last dg-do won, regardless of whether
the dg-do was selected or deselected.
I suppose the question is whether we can reasonably assume that people
are using dejagnu 1.6+ or whether we need to support older dejagnus.
It looks like Alex added dg-do-if (in e6f5fadec5f6a719145ed2ed513209ec3e8eeb2f)
to support older dejagnu, so that's an option. I.e.:
/* { dg-do compile } */
/* { dg-do-if assemble { target aarch64_asm_cmpbr_ok } } */
Which if it works (haven't tried!) also avoids having to specify the
selector twice. Not sure whether it's worth going back and changing
all existing aarch64 tests to this style though.
TIL :) Thanks Richard for bringing it up.
Richard
