rs6000/test: Update some cases with -mdejagnu-tune

  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
  

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