Re: [PATCH] rs6000, altivec-2-runnable.c update the require-effective-target
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
Re: [PATCH] rs6000, altivec-2-runnable.c update the require-effective-target
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
>>> 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
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
>> 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
On 6/14/24 1:37 PM, Carl Love wrote:
> Per the additional feedback after patch:
>
> commit c892525813c94b018464d5a4edc17f79186606b7
> Author: Carl Love
> 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
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
[PATCH] rs6000, altivec-2-runnable.c update the require-effective-target
GCC maintainers:
Per the additional feedback after patch:
commit c892525813c94b018464d5a4edc17f79186606b7
Author: Carl Love
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
--
2.45.0
