| Message ID | c2ff1021-e83f-6200-389e-df82e628af23@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PR,testsuite/81010] Fix PPC test | expand |
Hi! On Sun, Jan 07, 2018 at 12:58:25AM -0700, Jeff Law wrote: > As you note in the comments, the code we generate now is actually more > efficient so the test needs to be tweaked. > > Rather than checking the form in doloop, I check the form in .combine > and look for > > (compare:CC (zero_extend:DI (reg:SI > > The test is also twiddled to run on ppc64le. > > OK for the trunk? > > Jeff > PR testsuite/81010 > * gcc.target/powerpc/pr56605.c: Run on ppc64le too. Verify > the zero extension is part of the test in the combiner dump. > > > > diff --git a/gcc/testsuite/gcc.target/powerpc/pr56605.c b/gcc/testsuite/gcc.target/powerpc/pr56605.c > index 3bc335f..be6456c 100644 > --- a/gcc/testsuite/gcc.target/powerpc/pr56605.c > +++ b/gcc/testsuite/gcc.target/powerpc/pr56605.c > @@ -1,7 +1,7 @@ > /* PR rtl-optimization/56605 */ > -/* { dg-do compile { target { powerpc64-*-* && lp64 } } } */ > +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */ You could as well do powerpc*-*-* afaics? > /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */ And you could delete this line, since nothing in the testcase _needs_ the -mcpu=power7. > -/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-loop2_doloop" } */ > +/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-combine" } */ Something should check powerpc_vsx_ok, hrm. Or just remove -mvsx? > > void foo (short* __restrict sb, int* __restrict ia) > { > @@ -10,4 +10,4 @@ void foo (short* __restrict sb, int* __restrict ia) > ia[i] = (int) sb[i]; > } > > -/* { dg-final { scan-rtl-dump-times "\\\(compare:CC \\\(subreg:SI \\\(reg:DI" 1 "loop2_doloop" } } */ > +/* { dg-final { scan-rtl-dump-times "\\\(compare:CC \\\(zero_extend:DI \\\(reg:SI" 1 "combine" } } */ Yay, toothpickeritus ;-) But none of that is new, so okay with or without those extra cleanups. Thanks! Segher
Hi, On Sun, Jan 07, 2018 at 11:09:33AM -0600, Segher Boessenkool wrote: > > > /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */ > > And you could delete this line, since nothing in the testcase _needs_ the > -mcpu=power7. Scratch that, those overrides will be applied *later*. But do we need -mcpu=power7 at all anymore? And -mvsx? Oh well, fine either way. But it feels like the testcase does not really test anything anymore (nothing related to the original problem). Segher
On 01/07/2018 10:09 AM, Segher Boessenkool wrote: > Hi! > > On Sun, Jan 07, 2018 at 12:58:25AM -0700, Jeff Law wrote: >> As you note in the comments, the code we generate now is actually more >> efficient so the test needs to be tweaked. >> >> Rather than checking the form in doloop, I check the form in .combine >> and look for >> >> (compare:CC (zero_extend:DI (reg:SI >> >> The test is also twiddled to run on ppc64le. >> >> OK for the trunk? >> >> Jeff > >> PR testsuite/81010 >> * gcc.target/powerpc/pr56605.c: Run on ppc64le too. Verify >> the zero extension is part of the test in the combiner dump. >> >> >> >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr56605.c b/gcc/testsuite/gcc.target/powerpc/pr56605.c >> index 3bc335f..be6456c 100644 >> --- a/gcc/testsuite/gcc.target/powerpc/pr56605.c >> +++ b/gcc/testsuite/gcc.target/powerpc/pr56605.c >> @@ -1,7 +1,7 @@ >> /* PR rtl-optimization/56605 */ >> -/* { dg-do compile { target { powerpc64-*-* && lp64 } } } */ >> +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */ > > You could as well do powerpc*-*-* afaics? I guess, but I suspect the lp64 test would prevent it from running on the 32bit configurations anyway. And we probably are reliant on 64bits to expose the problematical conversions that we weren't handling well which led to 56605. > >> /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */ > > And you could delete this line, since nothing in the testcase _needs_ the > -mcpu=power7. Your later message indicates we should leave this as-is. So that's my plan here. > >> -/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-loop2_doloop" } */ >> +/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-combine" } */ > > Something should check powerpc_vsx_ok, hrm. Or just remove -mvsx? I think we should check powerpc_vsx_ok -- If I understand 56605 correctly ivopts/vectorization created the problematical gimple that when converted to RTL wasn't handled well. So we probably want to run the test on any 64bit ppc with VSX capabilities. WRT whether or not the test has any value, it still seems to be testing for whether or not we're able to simplify stuff appropriately, so I think keeping it probably makes the most sense. RTL testing/scanning is painful. I'll do a bit of sniff testing around the suggestions you've made and commit something reasonable :-) Jeff
On 01/07/2018 10:09 AM, Segher Boessenkool wrote: > Hi! > > On Sun, Jan 07, 2018 at 12:58:25AM -0700, Jeff Law wrote: >> As you note in the comments, the code we generate now is actually more >> efficient so the test needs to be tweaked. >> >> Rather than checking the form in doloop, I check the form in .combine >> and look for >> >> (compare:CC (zero_extend:DI (reg:SI >> >> The test is also twiddled to run on ppc64le. >> >> OK for the trunk? >> >> Jeff > >> PR testsuite/81010 >> * gcc.target/powerpc/pr56605.c: Run on ppc64le too. Verify >> the zero extension is part of the test in the combiner dump. >> >> >> >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr56605.c b/gcc/testsuite/gcc.target/powerpc/pr56605.c >> index 3bc335f..be6456c 100644 >> --- a/gcc/testsuite/gcc.target/powerpc/pr56605.c >> +++ b/gcc/testsuite/gcc.target/powerpc/pr56605.c >> @@ -1,7 +1,7 @@ >> /* PR rtl-optimization/56605 */ >> -/* { dg-do compile { target { powerpc64-*-* && lp64 } } } */ >> +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */ > > You could as well do powerpc*-*-* afaics? > >> /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */ > > And you could delete this line, since nothing in the testcase _needs_ the > -mcpu=power7. > >> -/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-loop2_doloop" } */ >> +/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-combine" } */ > > Something should check powerpc_vsx_ok, hrm. Or just remove -mvsx? > >> >> void foo (short* __restrict sb, int* __restrict ia) >> { >> @@ -10,4 +10,4 @@ void foo (short* __restrict sb, int* __restrict ia) >> ia[i] = (int) sb[i]; >> } >> >> -/* { dg-final { scan-rtl-dump-times "\\\(compare:CC \\\(subreg:SI \\\(reg:DI" 1 "loop2_doloop" } } */ >> +/* { dg-final { scan-rtl-dump-times "\\\(compare:CC \\\(zero_extend:DI \\\(reg:SI" 1 "combine" } } */ > > Yay, toothpickeritus ;-) > > But none of that is new, so okay with or without those extra cleanups. I've implemented most of the cleanups suggested. Essentially bringing the dg selectors/options in line with other vsx tests. As expected this is still unsupported on ppc32. Also as expected if I force it to run, it would fail because we don't have the problematical conversions between SImode and DImode. And (of course) we pass the updated test on ppc64 and ppc64le. Attached is the final version of the patch that I've installed. Jeff PR testsuite/81010 * gcc.target/powerpc/pr56605.c: Update various dg- directives to better match other tests which require vsx. Verify the zero extension is part of the test in the combiner dump. diff --git a/gcc/testsuite/gcc.target/powerpc/pr56605.c b/gcc/testsuite/gcc.target/powerpc/pr56605.c index 3bc335f..dc87640 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr56605.c +++ b/gcc/testsuite/gcc.target/powerpc/pr56605.c @@ -1,7 +1,9 @@ /* PR rtl-optimization/56605 */ -/* { dg-do compile { target { powerpc64-*-* && lp64 } } } */ +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */ +/* { dg-skip-if "" { powerpc*-*-darwin* } } */ +/* { dg-require-effective-target powerpc_vsx_ok } */ /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */ -/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-loop2_doloop" } */ +/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-combine" } */ void foo (short* __restrict sb, int* __restrict ia) { @@ -10,4 +12,5 @@ void foo (short* __restrict sb, int* __restrict ia) ia[i] = (int) sb[i]; } -/* { dg-final { scan-rtl-dump-times "\\\(compare:CC \\\(subreg:SI \\\(reg:DI" 1 "loop2_doloop" } } */ +/* { dg-final { scan-rtl-dump-times "\\\(compare:CC \\\(zero_extend:DI \\\(reg:SI" 1 "combine" } } */ +
On Jan 29 2018, Jeff Law <law@redhat.com> wrote: > On 01/07/2018 10:09 AM, Segher Boessenkool wrote: >> Hi! >> >> On Sun, Jan 07, 2018 at 12:58:25AM -0700, Jeff Law wrote: >>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr56605.c b/gcc/testsuite/gcc.target/powerpc/pr56605.c >>> index 3bc335f..be6456c 100644 >>> --- a/gcc/testsuite/gcc.target/powerpc/pr56605.c >>> +++ b/gcc/testsuite/gcc.target/powerpc/pr56605.c >>> @@ -1,7 +1,7 @@ >>> /* PR rtl-optimization/56605 */ >>> -/* { dg-do compile { target { powerpc64-*-* && lp64 } } } */ >>> +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */ >> >> You could as well do powerpc*-*-* afaics? > I guess, but I suspect the lp64 test would prevent it from running on > the 32bit configurations anyway. -m64 will pass the lp64 test. Andreas.
On Mon, Jan 29, 2018 at 09:41:12PM -0700, Jeff Law wrote: > On 01/07/2018 10:09 AM, Segher Boessenkool wrote: > >> --- a/gcc/testsuite/gcc.target/powerpc/pr56605.c > >> +++ b/gcc/testsuite/gcc.target/powerpc/pr56605.c > >> @@ -1,7 +1,7 @@ > >> /* PR rtl-optimization/56605 */ > >> -/* { dg-do compile { target { powerpc64-*-* && lp64 } } } */ > >> +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */ > > > > You could as well do powerpc*-*-* afaics? > I guess, but I suspect the lp64 test would prevent it from running on > the 32bit configurations anyway. And we probably are reliant on 64bits > to expose the problematical conversions that we weren't handling well > which led to 56605. The point is that the target triple does not say if you are compiling for 32-bit or 64-bit. biarch... > I'll do a bit of sniff testing around the suggestions you've made and > commit something reasonable :-) Thanks :-) Segher
diff --git a/gcc/testsuite/gcc.target/powerpc/pr56605.c b/gcc/testsuite/gcc.target/powerpc/pr56605.c index 3bc335f..be6456c 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr56605.c +++ b/gcc/testsuite/gcc.target/powerpc/pr56605.c @@ -1,7 +1,7 @@ /* PR rtl-optimization/56605 */ -/* { dg-do compile { target { powerpc64-*-* && lp64 } } } */ +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */ /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */ -/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-loop2_doloop" } */ +/* { dg-options "-O3 -mvsx -mcpu=power7 -fno-unroll-loops -fdump-rtl-combine" } */ void foo (short* __restrict sb, int* __restrict ia) { @@ -10,4 +10,4 @@ void foo (short* __restrict sb, int* __restrict ia) ia[i] = (int) sb[i]; } -/* { dg-final { scan-rtl-dump-times "\\\(compare:CC \\\(subreg:SI \\\(reg:DI" 1 "loop2_doloop" } } */ +/* { dg-final { scan-rtl-dump-times "\\\(compare:CC \\\(zero_extend:DI \\\(reg:SI" 1 "combine" } } */