| Message ID | 600de562-39bf-4358-a128-2766fd775f4f@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000, altivec-2-runnable.c update the require-effective-target | expand |
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
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
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
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
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
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>
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(-)