Re: [PATCH] Fix tests for gomp

["Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com>]

On 13/12/2023 10:55, Jakub Jelinek wrote:
> On Wed, Dec 13, 2023 at 10:43:16AM +0000, Andre Vieira (lists) wrote:
>> Hi,
>>
>> Apologies for the delay and this mixup. I need to do something different
>>
>> This is to fix testisms initially introduced by:
>> commit f5fc001a84a7dbb942a6252b3162dd38b4aae311
>> Author: Andre Vieira <andre.simoesdiasvieira@arm.com>
>> Date:   Mon Dec 11 14:24:41 2023 +0000
>>
>>      aarch64: enable mixed-types for aarch64 simdclones
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* gcc.dg/gomp/pr87887-1.c: Fixed test.
>> 	* gcc.dg/gomp/pr89246-1.c: Likewise.
>> 	* gcc.dg/gomp/simd-clones-2.c: Likewise.
>>
>> libgomp/ChangeLog:
>>
>> 	* testsuite/libgomp.c/declare-variant-1.c: Fixed test.
>> 	* testsuite/libgomp.fortran/declare-simd-1.f90: Likewise.
>>
>> OK for trunk? I was intending to commit as obvious, but jakub had made a
>> comment about declare-simd-1.f90 so I thought it might be worth just sending
>> it up to the mailing list first.
> 
>> --- a/libgomp/testsuite/libgomp.c/declare-variant-1.c
>> +++ b/libgomp/testsuite/libgomp.c/declare-variant-1.c
>> @@ -40,16 +40,17 @@ f04 (int a)
>>   int
>>   test1 (int x)
>>   {
>> -  /* At gimplification time, we can't decide yet which function to call.  */
>> -  /* { dg-final { scan-tree-dump-times "f04 \\\(x" 2 "gimple" } } */
>> +  /* At gimplification time, we can't decide yet which function to call for
>> +     x86_64 targets, given the f01 variant.  */
>> +  /* { dg-final { scan-tree-dump-times "f04 \\\(x" 2 "gimple" { target x86_64-*-* } } } */
>>     /* After simd clones are created, the original non-clone test1 shall
>>        call f03 (score 6), the sse2/avx/avx2 clones too, but avx512f clones
>>        shall call f01 with score 8.  */
>>     /* { dg-final { scan-ltrans-tree-dump-not "f04 \\\(x" "optimized" } } */
>> -  /* { dg-final { scan-ltrans-tree-dump-times "f03 \\\(x" 14 "optimized" { target { !aarch64*-*-* } } } } } */
>> -  /* { dg-final { scan-ltrans-tree-dump-times "f01 \\\(x" 4 "optimized" { target { !aarch64*-*-* } } } } } */
>> -  /* { dg-final { scan-ltrans-tree-dump-times "f03 \\\(x" 10 "optimized" { target { aarch64*-*-* } } } } } */
>> -  /* { dg-final { scan-ltrans-tree-dump-not "f01 \\\(x" "optimized" { target { aarch64*-*-* } } } } } */
>> +  /* { dg-final { scan-ltrans-tree-dump-times "f03 \\\(x" 14 "optimized" { target { !aarch64*-*-* } } } } */
>> +  /* { dg-final { scan-ltrans-tree-dump-times "f01 \\\(x" 4 "optimized" { target { !aarch64*-*-* } } } } */
>> +  /* { dg-final { scan-ltrans-tree-dump-times "f03 \\\(x" 10 "optimized" { target { aarch64*-*-* } } } } */
>> +  /* { dg-final { scan-ltrans-tree-dump-not "f01 \\\(x" "optimized" { target { aarch64*-*-* } } } } */
> 
> The changes in this test look all wrong.  The differences are
> i?86-*-* x86_64-*-* (which can support avx512f isa) vs. other targets (which
> can't).
> So, there is nothing aarch64 specific in there and { target x86_64-*-* }
> is also incorrect.  It should be simply
> { target i?86-*-* x86_64-*-* }
> vs.
> { target { ! { i?86-*-* x86_64-*-* } } }
> (never sure about the ! syntaxes).
> 

Hmm.... I think I understand what you are saying, but I'm not sure I 
agree. So before I enabled simdclone testing for aarch64, this test had 
no target selectors. So it checked the same for 'all simdclone test 
targets'. Which seem to be x86 and amdgcn:

@@ -4321,7 +4321,8 @@ proc check_effective_target_vect_simd_clones { } {
      return [check_cached_effective_target_indexed vect_simd_clones {
        expr { (([istarget i?86-*-*] || [istarget x86_64-*-*])
               && [check_effective_target_avx512f])
             || [istarget amdgcn-*-*]
             || [istarget aarch64*-*-*] }}]
  }

I haven't checked what amdgcn does with this test, but I'd have to 
assume they were passing before? Though I'm not sure how amdgcn would 
pass the original:
  -  /* At gimplification time, we can't decide yet which function to 
call.  */
  -  /* { dg-final { scan-tree-dump-times "f04 \\\(x" 2 "gimple" } } */

I've added Andrew to the mail to see if he can comment on that. Either 
way I'd suggest we either add scan's per target with the expected value 
or stick with my original change of aarch64 vs non-aarch64 as I think 
that would better reflect the changes of enabling this for aarch64 where 
it wasn't ran before.