[PATCH] rs6000, altivec-2-runnable.c update the require-effective-target

[PATCH] rs6000, altivec-2-runnable.c update the require-effective-target

[Carl Love]

GCC maintainers:

Per the additional feedback after patch: 

  commit c892525813c94b018464d5a4edc17f79186606b7
  Author: Carl Love <cel@linux.ibm.com>
  Date:   Tue Jun 11 14:01:16 2024 -0400

      rs6000, altivec-2-runnable.c should be a runnable test
    
      The test case has "dg-do compile" set not "dg-do run" for a runnable
      test.  This patch changes the dg-do command argument to run.
    
      gcc/testsuite/ChangeLog:gcc/testsuite/ChangeLog:
              * gcc.target/powerpc/altivec-2-runnable.c: Change dg-do
              argument to run.

was approved and committed, I have updated the dg-require-effective-target
and dg-options as requested so the test will compile with -O2 on a 
machine that has a minimum support of Power 8 vector hardware.

The patch has been tested on Power 10 with no regression failures.

Please let me know if this patch is acceptable for mainline.  Thanks.

                        Carl 

------------------------------------------------------------------

rs6000, altivec-2-runnable.c update the require-effective-target

The test requires a minimum of Power8 vector HW and a compile level
of -O2.

gcc/testsuite/ChangeLog:gcc/testsuite/ChangeLog:
	* gcc.target/powerpc/altivec-2-runnable.c: Change the
	require-effective-target for the test.
---
 gcc/testsuite/gcc.target/powerpc/altivec-2-runnable.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gcc/testsuite/gcc.target/powerpc/altivec-2-runnable.c b/gcc/testsuite/gcc.target/powerpc/altivec-2-runnable.c
index 17b23eb9d50..04c7d1ac70e 100644
--- a/gcc/testsuite/gcc.target/powerpc/altivec-2-runnable.c
+++ b/gcc/testsuite/gcc.target/powerpc/altivec-2-runnable.c
@@ -1,7 +1,6 @@
 /* { dg-do run } */
-/* { dg-options "-mvsx" } */
-/* { dg-additional-options "-mdejagnu-cpu=power8" { target { ! has_arch_pwr8 } } } */
-/* { dg-require-effective-target powerpc_vsx } */
+/* { dg-options "-O2 -mdejagnu-cpu=power8" } */
+/* { dg-require-effective-target p8vector_hw } */
 
 #include <altivec.h>
 
-- 
2.45.0

Re: [PATCH] rs6000, altivec-2-runnable.c update the require-effective-target

[Segher Boessenkool]

Hi!

On Fri, Jun 14, 2024 at 11:37:46AM -0700, Carl Love wrote:
>  /* { dg-do run } */
> -/* { dg-options "-mvsx" } */
> -/* { dg-additional-options "-mdejagnu-cpu=power8" { target { ! has_arch_pwr8 } } } */
> -/* { dg-require-effective-target powerpc_vsx } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power8" } */
> +/* { dg-require-effective-target p8vector_hw } */

I have no idea why the original didn't do -O2 already, heh.  So this is
only an improvement, right!  I won't complain at all unless it fails :-)


Segher

Re: [PATCH] rs6000, altivec-2-runnable.c update the require-effective-target

[Peter Bergner]

On 6/14/24 1:37 PM, Carl Love wrote:
> Per the additional feedback after patch: 
> 
>   commit c892525813c94b018464d5a4edc17f79186606b7
>   Author: Carl Love <cel@linux.ibm.com>
>   Date:   Tue Jun 11 14:01:16 2024 -0400
> 
>       rs6000, altivec-2-runnable.c should be a runnable test
>     
>       The test case has "dg-do compile" set not "dg-do run" for a runnable
>       test.  This patch changes the dg-do command argument to run.
>     
>       gcc/testsuite/ChangeLog:gcc/testsuite/ChangeLog:
>               * gcc.target/powerpc/altivec-2-runnable.c: Change dg-do
>               argument to run.

Test case altivec-1-runnable.c seems to have the same issue, in that it
is currently a dg-do compile test case rather than the intended dg-do run.
Can you have a look at changing that to dg-do run too?  My guess it that
this one will want something similar to some other altivec test cases, ala:

/* { dg-do run { target vmx_hw } } */
/* { dg-do compile { target { ! vmx_hw } } } */
/* { dg-require-effective-target powerpc_altivec_ok } */
/* { dg-options "-O2 -maltivec -mabi=altivec" } */


That said, I don't like not having a -mdejagnu-cpu=... here.
I think for our server cpus, this is fine, but on an embedded system
with a old ISA default for -mcpu=... (so we be doing a dg-do compile),
just adding -maltivec to that default may not make much sense for that
default and probably should be an error.  Maybe something like:

/* { dg-do run { target vmx_hw } } */
/* { dg-do compile { target { ! vmx_hw } } } */
/* { dg-require-effective-target powerpc_altivec_ok } */
/* { dg-options "-O2 -mdejagnu=power7" } */

...makes more sense?   Ke Wen & Segher, thoughts on that?
Ke Wen, should powerpc_altivec_ok be powerpc_altivec here???

Peter

Re: [PATCH] rs6000, altivec-2-runnable.c update the require-effective-target

[Kewen.Lin]

Hi,

on 2024/6/18 00:08, Peter Bergner wrote:
> On 6/14/24 1:37 PM, Carl Love wrote:
>> Per the additional feedback after patch: 
>>
>>   commit c892525813c94b018464d5a4edc17f79186606b7
>>   Author: Carl Love <cel@linux.ibm.com>
>>   Date:   Tue Jun 11 14:01:16 2024 -0400
>>
>>       rs6000, altivec-2-runnable.c should be a runnable test
>>     
>>       The test case has "dg-do compile" set not "dg-do run" for a runnable
>>       test.  This patch changes the dg-do command argument to run.
>>     
>>       gcc/testsuite/ChangeLog:gcc/testsuite/ChangeLog:
>>               * gcc.target/powerpc/altivec-2-runnable.c: Change dg-do
>>               argument to run.
> 
> Test case altivec-1-runnable.c seems to have the same issue, in that it
> is currently a dg-do compile test case rather than the intended dg-do run.

Good catch!

> Can you have a look at changing that to dg-do run too?  My guess it that
> this one will want something similar to some other altivec test cases, ala:
> 
> /* { dg-do run { target vmx_hw } } */
> /* { dg-do compile { target { ! vmx_hw } } } */
> /* { dg-require-effective-target powerpc_altivec_ok } */
> /* { dg-options "-O2 -maltivec -mabi=altivec" } */

I'd expect the "-runnable" test case focuses on testing for run.  Normally,
the one without "-runnable" would focus on testing for compiling (scan some
desired insn), but this altivec-1.c and altivec-1-runnable.c seems to test
for different things, maybe we should separate them into different names
if they don't test for a same test point.

> 
> That said, I don't like not having a -mdejagnu-cpu=... here.
> I think for our server cpus, this is fine, but on an embedded system
> with a old ISA default for -mcpu=... (so we be doing a dg-do compile),
> just adding -maltivec to that default may not make much sense for that
> default and probably should be an error.  Maybe something like:

Yes, for some embedded cpus, there will be some error messages, but since
we have powerpc_altivec_ok effective target, the error would make that
effective target checking fail so I'd expect it'll stop it being tested
(unsupported).

> 
> /* { dg-do run { target vmx_hw } } */
> /* { dg-do compile { target { ! vmx_hw } } } */
> /* { dg-require-effective-target powerpc_altivec_ok } */
> /* { dg-options "-O2 -mdejagnu=power7" } */
> 
> ...makes more sense?   Ke Wen & Segher, thoughts on that?
> Ke Wen, should powerpc_altivec_ok be powerpc_altivec here???

Yes, I just pushed r15-1390 for this change.

BR,
Kewen

Re: [PATCH] rs6000, altivec-2-runnable.c update the require-effective-target

[Carl Love]

Kewen, Peter, Segher:

On 6/17/24 19:56, Kewen.Lin wrote:
> Hi,
> 
> on 2024/6/18 00:08, Peter Bergner wrote:
>> On 6/14/24 1:37 PM, Carl Love wrote:
>>> Per the additional feedback after patch: 
>>>
>>>   commit c892525813c94b018464d5a4edc17f79186606b7
>>>   Author: Carl Love <cel@linux.ibm.com>
>>>   Date:   Tue Jun 11 14:01:16 2024 -0400
>>>
>>>       rs6000, altivec-2-runnable.c should be a runnable test
>>>     
>>>       The test case has "dg-do compile" set not "dg-do run" for a runnable
>>>       test.  This patch changes the dg-do command argument to run.
>>>     
>>>       gcc/testsuite/ChangeLog:gcc/testsuite/ChangeLog:
>>>               * gcc.target/powerpc/altivec-2-runnable.c: Change dg-do
>>>               argument to run.
>>
>> Test case altivec-1-runnable.c seems to have the same issue, in that it
>> is currently a dg-do compile test case rather than the intended dg-do run.
> 
> Good catch!

OK, will update that as well.  I think it will need the same header as altivec-2-runnable.c
so once we have a final change for altivec-2-runnable.c, I will make the header for
altivec-1-runnable.c be the same.

> 
>> Can you have a look at changing that to dg-do run too?  My guess it that
>> this one will want something similar to some other altivec test cases, ala:
>>
>> /* { dg-do run { target vmx_hw } } */
>> /* { dg-do compile { target { ! vmx_hw } } } */
>> /* { dg-require-effective-target powerpc_altivec_ok } */
>> /* { dg-options "-O2 -maltivec -mabi=altivec" } */
> 
> I'd expect the "-runnable" test case focuses on testing for run.  Normally,
> the one without "-runnable" would focus on testing for compiling (scan some
> desired insn), but this altivec-1.c and altivec-1-runnable.c seems to test
> for different things, maybe we should separate them into different names
> if they don't test for a same test point.

The altivec-1-runnable.c and altivec-2-runnable.c tests were added for various
built-ins that didn't have any test cases.  There wasn't an intention that there was 
any connection to the existing altivec-*.c test files.  I started creating runnable
when I started adding support for built-ins that we claimed to support but had never
actually been implemented.  I created runnable tests to make sure my implementation
actually worked.  I continued to add runnable tests for built-ins
that existed but didn't have a test case.  Adding runnable tests did find a couple
of issues where the existing implementation had a bug.  

That all said, if we want tochange the name of altivec-1-runnable.c and 
altivec-2-runnable.c a different naming scheme that is fine with me. Perhaps we should 
finish fixing the header for this test file, then do altivec-1-runnable, and then 
a final patch that does all the file renaming?

> 
>>
>> That said, I don't like not having a -mdejagnu-cpu=... here.
>> I think for our server cpus, this is fine, but on an embedded system
>> with a old ISA default for -mcpu=... (so we be doing a dg-do compile),
>> just adding -maltivec to that default may not make much sense for that
>> default and probably should be an error.  Maybe something like:
> 
> Yes, for some embedded cpus, there will be some error messages, but since
> we have powerpc_altivec_ok effective target, the error would make that
> effective target checking fail so I'd expect it'll stop it being tested
> (unsupported).
> 
>>
>> /* { dg-do run { target vmx_hw } } */
>> /* { dg-do compile { target { ! vmx_hw } } } */
>> /* { dg-require-effective-target powerpc_altivec_ok } */
>> /* { dg-options "-O2 -mdejagnu=power7" } */
>>
>> ...makes more sense?   Ke Wen & Segher, thoughts on that?
>> Ke Wen, should powerpc_altivec_ok be powerpc_altivec here???
> 
> Yes, I just pushed r15-1390 for this change.
> 
> BR,
> Kewen
> 

We had -mdejagnu=power8 before, but it looks like we want to go to power7 now.

It sounds like we want the following:

/* { dg-do run { target vmx_hw } } */
/* { dg-do compile { target { ! vmx_hw } } } */
/* { dg-options "-O2 -mdejagnu=power7" } */
/* { dg-require-effective-target powerpc_altivec } */

                     Carl 

Re: [PATCH] rs6000, altivec-2-runnable.c update the require-effective-target

[Kewen.Lin]

Hi Carl,

>> I'd expect the "-runnable" test case focuses on testing for run.  Normally,
>> the one without "-runnable" would focus on testing for compiling (scan some
>> desired insn), but this altivec-1.c and altivec-1-runnable.c seems to test
>> for different things, maybe we should separate them into different names
>> if they don't test for a same test point.
> 
> The altivec-1-runnable.c and altivec-2-runnable.c tests were added for various
> built-ins that didn't have any test cases.  There wasn't an intention that there was 
> any connection to the existing altivec-*.c test files.  I started creating runnable
> when I started adding support for built-ins that we claimed to support but had never
> actually been implemented.  I created runnable tests to make sure my implementation
> actually worked.  I continued to add runnable tests for built-ins
> that existed but didn't have a test case.  Adding runnable tests did find a couple
> of issues where the existing implementation had a bug.  
> 
> That all said, if we want tochange the name of altivec-1-runnable.c and 
> altivec-2-runnable.c a different naming scheme that is fine with me. Perhaps we should 
> finish fixing the header for this test file, then do altivec-1-runnable, and then 
> a final patch that does all the file renaming?

Yes, that's what I preferred, maybe something like altivec-run-n.c or
altivec-runnable-n.c to avoid the possible confusion.


>>> That said, I don't like not having a -mdejagnu-cpu=... here.
>>> I think for our server cpus, this is fine, but on an embedded system
>>> with a old ISA default for -mcpu=... (so we be doing a dg-do compile),
>>> just adding -maltivec to that default may not make much sense for that
>>> default and probably should be an error.  Maybe something like:
>>
>> Yes, for some embedded cpus, there will be some error messages, but since
>> we have powerpc_altivec_ok effective target, the error would make that
>> effective target checking fail so I'd expect it'll stop it being tested
>> (unsupported).
>>
>>>
>>> /* { dg-do run { target vmx_hw } } */
>>> /* { dg-do compile { target { ! vmx_hw } } } */
>>> /* { dg-require-effective-target powerpc_altivec_ok } */
>>> /* { dg-options "-O2 -mdejagnu=power7" } */
>>>

...

> We had -mdejagnu=power8 before, but it looks like we want to go to power7 now.
> 
> It sounds like we want the following:
> 
> /* { dg-do run { target vmx_hw } } */
> /* { dg-do compile { target { ! vmx_hw } } } */
> /* { dg-options "-O2 -mdejagnu=power7" } */
> /* { dg-require-effective-target powerpc_altivec } */

As mentioned above, I'd expect powerpc_altivec can stop this being tested
without altivec feature support, so IMHO an explicit cpu type isn't necessary
(though I'm not opposed to specifying it), btw, s/-mdejagnu/-mdejagnu-cpu/.

BR,
Kewen