rs6000/test: Update some cases with -mdejagnu-tune
Commit Message
Hi,
As PR106345 shows, some test cases should be updated with
-mdejagnu-tune, since their test points are sensitive to
rs6000_tune, such as: group_ending_nop, loop align (ic),
float conversion cost etc.
This patch is to replace -mdejagnu-cpu with -mdejagnu-tune
or append -mdejagnu-tune (keep the original -mdejagnu-cpu
when it's required) accordingly.
Tested on powerpc64-linux-gnu P7 and P8 and
powerpc64le-linux-gnu P9 and P10, also with explicit p10
tune setting for configuration.
I'll push this soon if no objections.
BR,
Kewen
-----
PR testsuite/106345
gcc/testsuite/ChangeLog:
* gcc.target/powerpc/lhs-1.c: Replace -mdejagnu-cpu with
-mdejagnu-tune.
* gcc.target/powerpc/loop_align.c: Likewise.
* gcc.target/powerpc/lhs-2.c: Append -mdejagnu-tune.
* gcc.target/powerpc/lhs-3.c: Likewise.
* gcc.target/powerpc/compress-float-ppc-pic.c: Likewise.
* gcc.target/powerpc/compress-float-ppc.c: Likewise.
---
gcc/testsuite/gcc.target/powerpc/compress-float-ppc-pic.c | 2 +-
gcc/testsuite/gcc.target/powerpc/compress-float-ppc.c | 2 +-
gcc/testsuite/gcc.target/powerpc/lhs-1.c | 2 +-
gcc/testsuite/gcc.target/powerpc/lhs-2.c | 2 +-
gcc/testsuite/gcc.target/powerpc/lhs-3.c | 2 +-
gcc/testsuite/gcc.target/powerpc/loop_align.c | 2 +-
6 files changed, 6 insertions(+), 6 deletions(-)
--
2.27.0
Comments
Hi!
On Wed, Jul 20, 2022 at 05:31:11PM +0800, Kewen.Lin wrote:
> As PR106345 shows, some test cases should be updated with
> -mdejagnu-tune, since their test points are sensitive to
> rs6000_tune, such as: group_ending_nop, loop align (ic),
> float conversion cost etc.
It does not make sense to require -mdejagnu-tune= if -mdejagnu-cpu= is
already given? What is the failure case?
> This patch is to replace -mdejagnu-cpu with -mdejagnu-tune
> or append -mdejagnu-tune (keep the original -mdejagnu-cpu
> when it's required) accordingly.
It is *always* required. Testcases with -mtune= but unspecified -mcpu=
make no sense.
> --- a/gcc/testsuite/gcc.target/powerpc/compress-float-ppc-pic.c
> +++ b/gcc/testsuite/gcc.target/powerpc/compress-float-ppc-pic.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile { target powerpc_fprs } } */
> -/* { dg-options "-O2 -fpic -mdejagnu-cpu=power5" } */
> +/* { dg-options "-O2 -fpic -mdejagnu-cpu=power5 -mdejagnu-tune=power5" } */
> /* { dg-require-effective-target fpic } */
This should only make a difference if you have -mtune= in your
RUNTEST_FLAGS, and you shouldn't do silly things like that. I suspect
you see it in other cases, and those are actual bugs then, that need
actual fixing instead of sweeping under the carper.
The testcase suggests this is with a compiler configured with
--with-cpu= --with-tune=, which should just work, and -mcpu= should
override both of those!
Segher
Hi Segher,
Thanks for the comments!
on 2022/7/22 02:48, Segher Boessenkool wrote:
> Hi!
>
> On Wed, Jul 20, 2022 at 05:31:11PM +0800, Kewen.Lin wrote:
>> As PR106345 shows, some test cases should be updated with
>> -mdejagnu-tune, since their test points are sensitive to
>> rs6000_tune, such as: group_ending_nop, loop align (ic),
>> float conversion cost etc.
>
> It does not make sense to require -mdejagnu-tune= if -mdejagnu-cpu= is
> already given? What is the failure case?
>
I think cpu setting only sets tune setting when tune setting isn't
explicitly provided as:
if (rs6000_tune_index >= 0)
tune_index = rs6000_tune_index;
else if (cpu_index >= 0)
rs6000_tune_index = tune_index = cpu_index;
As PR106345 shows, GCC can use an explicit tune setting when it's
configured, even if there is one "-mdejagnu-cpu=", it doesn't
override the explicit given one, so we need one explicit
"-mdejagnu-tune=".
One failure example is gcc.target/powerpc/loop_align.c
See function rs6000_loop_align:
/* Implement LOOP_ALIGN. */
align_flags
rs6000_loop_align (rtx label)
{
...
/* Align small loops to 32 bytes to fit in an icache sector, otherwise return default. */
if (ninsns > 4 && ninsns <= 8
&& (rs6000_tune == PROCESSOR_POWER4
|| rs6000_tune == PROCESSOR_POWER5
|| rs6000_tune == PROCESSOR_POWER6
|| rs6000_tune == PROCESSOR_POWER7
|| rs6000_tune == PROCESSOR_POWER8))
return align_flags (5);
else
return align_loops;
Although the test case has adopted option "-mdejagnu-cpu=power7", but
the configured "--with-tune-64=power9" takes effect and make it
return align_loops instead of align_flags (5).
>> This patch is to replace -mdejagnu-cpu with -mdejagnu-tune
>> or append -mdejagnu-tune (keep the original -mdejagnu-cpu
>> when it's required) accordingly.
>
> It is *always* required. Testcases with -mtune= but unspecified -mcpu=
> make no sense.
>
The loop_align.c testings made me think if we know the insn count for
the loop on all cpus is in range (4,8] then the cpu setting doesn't matter.
I think I get your point, it's risky to assume that even if it works
for all existing cpus, will update with an explicit -mdejagnu-cpu here.
>> --- a/gcc/testsuite/gcc.target/powerpc/compress-float-ppc-pic.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/compress-float-ppc-pic.c
>> @@ -1,5 +1,5 @@
>> /* { dg-do compile { target powerpc_fprs } } */
>> -/* { dg-options "-O2 -fpic -mdejagnu-cpu=power5" } */
>> +/* { dg-options "-O2 -fpic -mdejagnu-cpu=power5 -mdejagnu-tune=power5" } */
>> /* { dg-require-effective-target fpic } */
>
> This should only make a difference if you have -mtune= in your
> RUNTEST_FLAGS, and you shouldn't do silly things like that. I suspect
> you see it in other cases, and those are actual bugs then, that need
> actual fixing instead of sweeping under the carper.
>
Unfortunately it's due to the explicit tune setting in configuration.
> The testcase suggests this is with a compiler configured with
> --with-cpu= --with-tune=, which should just work, and -mcpu= should
> override both of those!
>
Unfortunately -mcpu= (-mdejagnu-cpu=) doesn't actually override here.
BR,
Kewen
On Fri, Jul 22, 2022 at 10:22:51AM +0800, Kewen.Lin wrote:
> on 2022/7/22 02:48, Segher Boessenkool wrote:
> > On Wed, Jul 20, 2022 at 05:31:11PM +0800, Kewen.Lin wrote:
> >> As PR106345 shows, some test cases should be updated with
> >> -mdejagnu-tune, since their test points are sensitive to
> >> rs6000_tune, such as: group_ending_nop, loop align (ic),
> >> float conversion cost etc.
> >
> > It does not make sense to require -mdejagnu-tune= if -mdejagnu-cpu= is
> > already given? What is the failure case?
> >
>
> I think cpu setting only sets tune setting when tune setting isn't
> explicitly provided as:
>
> if (rs6000_tune_index >= 0)
> tune_index = rs6000_tune_index;
> else if (cpu_index >= 0)
> rs6000_tune_index = tune_index = cpu_index;
>
> As PR106345 shows, GCC can use an explicit tune setting when it's
> configured, even if there is one "-mdejagnu-cpu=", it doesn't
> override the explicit given one, so we need one explicit
> "-mdejagnu-tune=".
And that is the problem. GCC's automatic setting is *not* an explicit
option, not given by the user. --with-tune= should not result in adding
an -mtune= option in the resulting compiler, it should not set command-
line options.
> Although the test case has adopted option "-mdejagnu-cpu=power7", but
> the configured "--with-tune-64=power9" takes effect and make it
> return align_loops instead of align_flags (5).
And it should not do that.
> >> This patch is to replace -mdejagnu-cpu with -mdejagnu-tune
> >> or append -mdejagnu-tune (keep the original -mdejagnu-cpu
> >> when it's required) accordingly.
> >
> > It is *always* required. Testcases with -mtune= but unspecified -mcpu=
> > make no sense.
>
> The loop_align.c testings made me think if we know the insn count for
> the loop on all cpus is in range (4,8] then the cpu setting doesn't matter.
Sure, it probably works without -mcpu=, but it does not make sense :-)
Only using -mtune= while not having -mcpu= serves no purpose in any
"normal" use, so we shouldn't do that in the testsuite either.
> > This should only make a difference if you have -mtune= in your
> > RUNTEST_FLAGS, and you shouldn't do silly things like that. I suspect
> > you see it in other cases, and those are actual bugs then, that need
> > actual fixing instead of sweeping under the carper.
>
> Unfortunately it's due to the explicit tune setting in configuration.
So that needs some actual fixes. Something in how --with-tune= works
is suboptimal?
> > The testcase suggests this is with a compiler configured with
> > --with-cpu= --with-tune=, which should just work, and -mcpu= should
> > override both of those!
>
> Unfortunately -mcpu= (-mdejagnu-cpu=) doesn't actually override here.
... or that.
Segher
On 7/22/22 1:17 PM, Segher Boessenkool wrote:
> On Fri, Jul 22, 2022 at 10:22:51AM +0800, Kewen.Lin wrote:
>> on 2022/7/22 02:48, Segher Boessenkool wrote:
>> As PR106345 shows, GCC can use an explicit tune setting when it's
>> configured, even if there is one "-mdejagnu-cpu=", it doesn't
>> override the explicit given one, so we need one explicit
>> "-mdejagnu-tune=".
>
> And that is the problem. GCC's automatic setting is *not* an explicit
> option, not given by the user. --with-tune= should not result in adding
> an -mtune= option in the resulting compiler, it should not set command-
> line options.
>
[snip]
> And it should not do that.
Currently, our rs6000.h has this:
/* Only for use in the testsuite: -mdejagnu-cpu= simply overrides -mcpu=.
With older versions of Dejagnu the command line arguments you set in
RUNTESTFLAGS override those set in the testcases; with this option,
the testcase will always win. Ditto for -mdejagnu-tune=. */
#define DRIVER_SELF_SPECS \
"%{mdejagnu-cpu=*: %<mcpu=* -mcpu=%*}", \
"%{mdejagnu-tune=*: %<mtune=* -mtune=%*}", \
"%{mdejagnu-*: %<mdejagnu-*}", \
SUBTARGET_DRIVER_SELF_SPECS
This means -mdejagnu-cpu= usage will only filter out -mcpu= usage.
I guess it should also filter out all -mtune= usage as well.
That way, we'll tune according to the -mdejagnu-cpu= option which is
what we want. That was an oversight on my part when I added the code. :-(
So I think the way the code above *should* work is:
1) Any -mdejagnu-cpu= usage should filter out all -mcpu= and -mtune= options.
2) Any -mdejagnu-tune= usage should filter all -mtune= options.
It should not filter out any -mcpu= options.
I believe that should fix the issue we're seeing.
Peter
On 7/22/22 1:53 PM, Peter Bergner wrote:
> So I think the way the code above *should* work is:
> 1) Any -mdejagnu-cpu= usage should filter out all -mcpu= and -mtune= options.
> 2) Any -mdejagnu-tune= usage should filter all -mtune= options.
> It should not filter out any -mcpu= options.
Like this:
diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
index 3b8941a8658..26874943795 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -86,7 +86,7 @@
RUNTESTFLAGS override those set in the testcases; with this option,
the testcase will always win. Ditto for -mdejagnu-tune=. */
#define DRIVER_SELF_SPECS \
- "%{mdejagnu-cpu=*: %<mcpu=* -mcpu=%*}", \
+ "%{mdejagnu-cpu=*: %<mcpu=* %<mtune=* -mcpu=%*}", \
"%{mdejagnu-tune=*: %<mtune=* -mtune=%*}", \
"%{mdejagnu-*: %<mdejagnu-*}", \
SUBTARGET_DRIVER_SELF_SPECS
Kewen, can you see if the above patch fixes the issues you're seeing?
Peter
Hi Peter and Segher,
on 2022/7/23 03:28, Peter Bergner wrote:
> On 7/22/22 1:53 PM, Peter Bergner wrote:
>> So I think the way the code above *should* work is:
>> 1) Any -mdejagnu-cpu= usage should filter out all -mcpu= and -mtune= options.
>> 2) Any -mdejagnu-tune= usage should filter all -mtune= options.
>> It should not filter out any -mcpu= options.
>
> Like this:
>
> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
> index 3b8941a8658..26874943795 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -86,7 +86,7 @@
> RUNTESTFLAGS override those set in the testcases; with this option,
> the testcase will always win. Ditto for -mdejagnu-tune=. */
> #define DRIVER_SELF_SPECS \
> - "%{mdejagnu-cpu=*: %<mcpu=* -mcpu=%*}", \
> + "%{mdejagnu-cpu=*: %<mcpu=* %<mtune=* -mcpu=%*}", \
> "%{mdejagnu-tune=*: %<mtune=* -mtune=%*}", \
> "%{mdejagnu-*: %<mdejagnu-*}", \
> SUBTARGET_DRIVER_SELF_SPECS
>
>
> Kewen, can you see if the above patch fixes the issues you're seeing?
>
Thanks for the insightful comments and patch!
I confirmed that this proposed patch can fix those found test issues.
I adjusted the relevant comments and confirmed that it can be bootstrapped and
regtested on powerpc64-linux-gnu P7 and P8 and powerpc64le-linux-gnu P9 and P10.
Segher pre-approved it, I just committed it as r13-1818 as attached.
Will backport it to release branches in a week or so. Thanks again.
BR,
Kewen
From 75d20d6c84c12bedd65a904e462f02f0b9eb3f77 Mon Sep 17 00:00:00 2001
From: Peter Bergner <bergner@linux.ibm.com>
Date: Mon, 25 Jul 2022 00:51:44 -0500
Subject: [PATCH] rs6000: Adjust -mdejagnu-cpu to filter out -mtune [PR106345]
As PR106345 shows, when configuring compiler with an explicit
option --with-tune=<value>, it would cause some test cases to
fail if their test points are sensitive to tune setting, such
as: group_ending_nop, loop align etc. It doesn't help that
even to specify one explicit -mcpu=.
This patch is to adjust the behavior of -mdejagnu-cpu by
filtering out all -mcpu= and -mtune= options, then test cases
would use <cpu> as tune as the one specified by -mdejagnu-cpu.
2022-07-25 Peter Bergner <bergner@linux.ibm.com>
Kewen Lin <linkw@linux.ibm.com>
PR testsuite/106345
gcc/ChangeLog:
* config/rs6000/rs6000.h (DRIVER_SELF_SPECS): Adjust -mdejagnu-cpu
to filter out all -mtune options.
---
gcc/config/rs6000/rs6000.h | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
index 7d04556304a..ad9bf0f7358 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -81,12 +81,15 @@
# define SUBTARGET_DRIVER_SELF_SPECS ""
#endif
-/* Only for use in the testsuite: -mdejagnu-cpu= simply overrides -mcpu=.
+/* Only for use in the testsuite: -mdejagnu-cpu=<value> filters out all
+ -mcpu= as well as -mtune= options then simply adds -mcpu=<value>,
+ while -mdejagnu-tune=<value> filters out all -mtune= options then
+ simply adds -mtune=<value>.
With older versions of Dejagnu the command line arguments you set in
- RUNTESTFLAGS override those set in the testcases; with this option,
- the testcase will always win. Ditto for -mdejagnu-tune=. */
+ RUNTESTFLAGS override those set in the testcases; with these options,
+ the testcase will always win. */
#define DRIVER_SELF_SPECS \
- "%{mdejagnu-cpu=*: %<mcpu=* -mcpu=%*}", \
+ "%{mdejagnu-cpu=*: %<mcpu=* %<mtune=* -mcpu=%*}", \
"%{mdejagnu-tune=*: %<mtune=* -mtune=%*}", \
"%{mdejagnu-*: %<mdejagnu-*}", \
SUBTARGET_DRIVER_SELF_SPECS
@@ -1,5 +1,5 @@
/* { dg-do compile { target powerpc_fprs } } */
-/* { dg-options "-O2 -fpic -mdejagnu-cpu=power5" } */
+/* { dg-options "-O2 -fpic -mdejagnu-cpu=power5 -mdejagnu-tune=power5" } */
/* { dg-require-effective-target fpic } */
double foo (double x) {
@@ -1,5 +1,5 @@
/* { dg-do compile { target powerpc_fprs } } */
-/* { dg-options "-O2 -mdejagnu-cpu=power5" } */
+/* { dg-options "-O2 -mdejagnu-cpu=power5 -mdejagnu-tune=power5" } */
double foo (double x) {
return x + 1.75;
@@ -1,6 +1,6 @@
/* { dg-do compile { target { powerpc*-*-* } } } */
/* { dg-skip-if "" { powerpc*-*-darwin* } } */
-/* { dg-options "-O2 -mdejagnu-cpu=power5" } */
+/* { dg-options "-O2 -mdejagnu-tune=power5" } */
/* { dg-final { scan-assembler-times "nop" 3 } } */
/* Test generation of nops in load hit store situation. Make sure enough nop
@@ -1,6 +1,6 @@
/* { dg-do compile { target { powerpc*-*-* } } } */
/* { dg-skip-if "" { powerpc*-*-darwin* } } */
-/* { dg-options "-O2 -mdejagnu-cpu=power6 -msched-groups" } */
+/* { dg-options "-O2 -mdejagnu-cpu=power6 -mdejagnu-tune=power6 -msched-groups" } */
/* { dg-final { scan-assembler "ori 1,1,0" } } */
/* Test generation of group ending nop in load hit store situation. */
@@ -1,6 +1,6 @@
/* { dg-do compile { target { powerpc*-*-* } } } */
/* { dg-skip-if "" { powerpc*-*-darwin* } } */
-/* { dg-options "-O2 -mdejagnu-cpu=power7" } */
+/* { dg-options "-O2 -mdejagnu-cpu=power7 -mdejagnu-tune=power7" } */
/* { dg-final { scan-assembler "ori 2,2,0" } } */
/* Test generation of group ending nop in load hit store situation. */
@@ -1,6 +1,6 @@
/* { dg-do compile { target { powerpc*-*-* } } } */
/* { dg-skip-if "" { powerpc*-*-darwin* powerpc-ibm-aix* } } */
-/* { dg-options "-O2 -mdejagnu-cpu=power7 -falign-functions=16 -fno-unroll-loops" } */
+/* { dg-options "-O2 -mdejagnu-tune=power7 -falign-functions=16 -fno-unroll-loops" } */
/* { dg-final { scan-assembler ".p2align 5" } } */
void f(double *a, double *b, double *c, unsigned long n) {