| Message ID | 20210714175205.GA4593@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | Repost #2: [PATCH] PR 100170: Fix eq/ne tests on power10. | expand |
Please do not send the same patches in a new thread. It is much more work to keep track of. Just ping patches by replying to them (either copy the list or not, either works). Thanks! Segher
On Wed, Jul 14, 2021 at 03:25:32PM -0500, Segher Boessenkool wrote: > Please do not send the same patches in a new thread. It is much more > work to keep track of. Just ping patches by replying to them (either > copy the list or not, either works). Thanks! Oh, and do not edit the Subject:. You managed to have the first 30 characters of it completely useless. You should never use more than 50 characters total, you use 57 already, although this should be an unusually *short* subject! (And subjects are not sentences, do not end in a full stop.) Segher
On Wed, Jul 14, 2021 at 01:52:05PM -0400, Michael Meissner wrote: > This patch updates eq/ne tests in the testsuite to adjust the test if > power10 code generation is used. eq0/ne0. > --- a/gcc/testsuite/gcc.target/powerpc/ppc-eq0-1.c > +++ b/gcc/testsuite/gcc.target/powerpc/ppc-eq0-1.c > -/* { dg-final { scan-assembler "cntlzw|isel" } } */ > +/* { dg-final { scan-assembler {\mcntlzw|isel|setbc\M} } } */ This does not do wha you perhaps think it does. It looks for one of the three atoms "\mcntlzw", "isel", or "setbc\M". You should write \m(cntlzw|isel|setbc)\M or, if you need it to not capture (like in a scan-assembler-times) \m(?:cntlzw|isel|setbc)\M > --- a/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c > +++ b/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c > -/* { dg-final { scan-assembler-times "addic" 4 } } */ > -/* { dg-final { scan-assembler-times "subfe" 1 } } */ > -/* { dg-final { scan-assembler-times "addze" 3 } } */ > +/* { dg-final { scan-assembler-times {\maddic\M} 4 { target { ! has_arch_pwr10 } } } } */ > +/* { dg-final { scan-assembler-times {\msubfe\M} 1 { target { ! has_arch_pwr10 } } } } */ > +/* { dg-final { scan-assembler-times {\maddic\M} 3 { target { has_arch_pwr10 } } } } */ > +/* { dg-final { scan-assembler-not {\msubfe\M} { target { has_arch_pwr10 } } } } */ > +/* { dg-final { scan-assembler-times {\msetbcr\M} 1 { target { has_arch_pwr10 } } } } */ > +/* { dg-final { scan-assembler-times {\maddze\M} 3 } } */ It may be easier to split the patch into two, where one part can get the setbcr (the first, simplest function), and the rest stays the same. Okay for trunk like that. Thanks! Segher
On Wed, Jul 14, 2021 at 04:22:06PM -0500, Segher Boessenkool wrote: > > --- a/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c > > +++ b/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c > > > -/* { dg-final { scan-assembler-times "addic" 4 } } */ > > -/* { dg-final { scan-assembler-times "subfe" 1 } } */ > > -/* { dg-final { scan-assembler-times "addze" 3 } } */ > > +/* { dg-final { scan-assembler-times {\maddic\M} 4 { target { ! has_arch_pwr10 } } } } */ > > +/* { dg-final { scan-assembler-times {\msubfe\M} 1 { target { ! has_arch_pwr10 } } } } */ > > +/* { dg-final { scan-assembler-times {\maddic\M} 3 { target { has_arch_pwr10 } } } } */ > > +/* { dg-final { scan-assembler-not {\msubfe\M} { target { has_arch_pwr10 } } } } */ > > +/* { dg-final { scan-assembler-times {\msetbcr\M} 1 { target { has_arch_pwr10 } } } } */ > > +/* { dg-final { scan-assembler-times {\maddze\M} 3 } } */ > > It may be easier to split the patch into two, where one part can get the > setbcr (the first, simplest function), and the rest stays the same. I really don't understand this comment. I don't see how you could split the patch in two, as the function that generates the setbcr (ne0) for power10 would generate addic/subfe instead of the setbcr on earlier power systems. Those instruction counts have to be changed for the other functions. So it doesn't make sense to split the patch to me.
On Mon, Jul 26, 2021 at 04:46:46PM -0400, Michael Meissner wrote: > On Wed, Jul 14, 2021 at 04:22:06PM -0500, Segher Boessenkool wrote: > > > --- a/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c > > > +++ b/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c > > > > > -/* { dg-final { scan-assembler-times "addic" 4 } } */ > > > -/* { dg-final { scan-assembler-times "subfe" 1 } } */ > > > -/* { dg-final { scan-assembler-times "addze" 3 } } */ > > > +/* { dg-final { scan-assembler-times {\maddic\M} 4 { target { ! has_arch_pwr10 } } } } */ > > > +/* { dg-final { scan-assembler-times {\msubfe\M} 1 { target { ! has_arch_pwr10 } } } } */ > > > +/* { dg-final { scan-assembler-times {\maddic\M} 3 { target { has_arch_pwr10 } } } } */ > > > +/* { dg-final { scan-assembler-not {\msubfe\M} { target { has_arch_pwr10 } } } } */ > > > +/* { dg-final { scan-assembler-times {\msetbcr\M} 1 { target { has_arch_pwr10 } } } } */ > > > +/* { dg-final { scan-assembler-times {\maddze\M} 3 } } */ > > > > It may be easier to split the patch into two, where one part can get the > > setbcr (the first, simplest function), and the rest stays the same. > > I really don't understand this comment. I don't see how you could split the > patch in two, as the function that generates the setbcr (ne0) for power10 would > generate addic/subfe instead of the setbcr on earlier power systems. Those > instruction counts have to be changed for the other functions. So it doesn't > make sense to split the patch to me. I'm sorry. I meant split the *testcase* into two :-) One with the first test, the other with the rest. Segher
diff --git a/gcc/testsuite/gcc.target/powerpc/ppc-eq0-1.c b/gcc/testsuite/gcc.target/powerpc/ppc-eq0-1.c index 496a6e340c0..bbdc7e00101 100644 --- a/gcc/testsuite/gcc.target/powerpc/ppc-eq0-1.c +++ b/gcc/testsuite/gcc.target/powerpc/ppc-eq0-1.c @@ -7,4 +7,4 @@ int foo(int x) return x == 0; } -/* { dg-final { scan-assembler "cntlzw|isel" } } */ +/* { dg-final { scan-assembler {\mcntlzw|isel|setbc\M} } } */ diff --git a/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c b/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c index 63c4b6087df..34c6de3874d 100644 --- a/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c +++ b/gcc/testsuite/gcc.target/powerpc/ppc-ne0-1.c @@ -2,9 +2,12 @@ /* { dg-do compile } */ /* { dg-options "-O2 -mno-isel" } */ -/* { dg-final { scan-assembler-times "addic" 4 } } */ -/* { dg-final { scan-assembler-times "subfe" 1 } } */ -/* { dg-final { scan-assembler-times "addze" 3 } } */ +/* { dg-final { scan-assembler-times {\maddic\M} 4 { target { ! has_arch_pwr10 } } } } */ +/* { dg-final { scan-assembler-times {\msubfe\M} 1 { target { ! has_arch_pwr10 } } } } */ +/* { dg-final { scan-assembler-times {\maddic\M} 3 { target { has_arch_pwr10 } } } } */ +/* { dg-final { scan-assembler-not {\msubfe\M} { target { has_arch_pwr10 } } } } */ +/* { dg-final { scan-assembler-times {\msetbcr\M} 1 { target { has_arch_pwr10 } } } } */ +/* { dg-final { scan-assembler-times {\maddze\M} 3 } } */ long ne0(long a) {