[PATCH] PR target/102059 Fix inline of target specific functions

[PATCH] PR target/102059 Fix inline of target specific functions

[Michael Meissner]

Reset -mpower8-fusion for power9 inlining power8 functions, PR 102059.

This patch is an attempt to make a much simpler patch to fix PR target/102059
than the previous patch.

It just fixes the issue that if a function is specifically declared as a power8
function, you can't inline in functions that are specified with power9 or
power10 options.

The issue is -mpower8-fusion is cleared when you use -mcpu=power9 or
-mcpu=power10.  When I wrote the code for controlling which function can inline
other functions, -mpower8-fusion was set for -mcpu=power9 (-mcpu=power10 was
not an option at that time).  This patch fixes this particular problem.

Perhaps -mpower8-fusion should go away in the GCC 13 time frame.  This patch
just goes in and resets the fusion bit for testing inlines.

I have built a bootstrapped little endian compiler on power9 and the tests had
no regressions.

I have built a bootstrapped big endian compiler on power8 and I tested both
32-bit and 64-bit builds, and there were no regressions.

Can I install this into the trunk and back port it into GCC 11 after a burn-in
period?

2022-02-08   Michael Meissner  <meissner@linux.ibm.com>

gcc/

	PR target/102059
	* config/rs6000/rs6000.cc (rs6000_can_inline_p): Don't test for
	power8-fusion if the caller was power9 or power10.

gcc/testsuite/
	PR target/102059
	* gcc.target/powerpc/pr102059-4.c: New test.
---
 gcc/config/rs6000/rs6000.cc                   |  8 ++++++++
 gcc/testsuite/gcc.target/powerpc/pr102059-4.c | 20 +++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr102059-4.c

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index eaba9a2d698..e2d94421ae9 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -25278,6 +25278,14 @@ rs6000_can_inline_p (tree caller, tree callee)
 	      callee_isa &= ~OPTION_MASK_HTM;
 	      explicit_isa &= ~OPTION_MASK_HTM;
 	    }
+
+	  /* Power9 and power10 do not set power8-fusion.  If the callee was
+	     explicitly compiled for power8, and the caller was power9 or
+	     power10, ignore the power8-fusion bits if it was set by
+	     default.  */
+	  if ((caller_isa & OPTION_MASK_P8_FUSION) == 0
+	      && (explicit_isa & OPTION_MASK_P8_FUSION) == 0)
+	    callee_isa &= ~OPTION_MASK_P8_FUSION;
 	}
 
       /* The callee's options must be a subset of the caller's options, i.e.
diff --git a/gcc/testsuite/gcc.target/powerpc/pr102059-4.c b/gcc/testsuite/gcc.target/powerpc/pr102059-4.c
new file mode 100644
index 00000000000..627c6f820c7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr102059-4.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mdejagnu-cpu=power10 -Wno-attributes" } */
+
+/* Verify that power10 can explicity include functions compiled for power8.
+   The issue is -mcpu=power8 enables -mpower8-fusion, but -mcpu=power9 or
+   -mcpu=power10 do not set power8-fusion by default.  */
+
+static inline int __attribute__ ((always_inline,target("cpu=power8")))
+foo (int *b)
+{
+  *b += 10;
+  return *b;
+}
+
+int
+bar (int *a)
+{
+  *a = foo (a);
+  return 0;
+}
-- 
2.34.1


-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: [PATCH] PR target/102059 Fix inline of target specific functions

[Kewen.Lin]

Hi Michael,

on 2022/2/9 上午11:27, Michael Meissner via Gcc-patches wrote:
> Reset -mpower8-fusion for power9 inlining power8 functions, PR 102059.
> 
> This patch is an attempt to make a much simpler patch to fix PR target/102059
> than the previous patch.
> 
> It just fixes the issue that if a function is specifically declared as a power8
> function, you can't inline in functions that are specified with power9 or
> power10 options.
> 
> The issue is -mpower8-fusion is cleared when you use -mcpu=power9 or
> -mcpu=power10.  When I wrote the code for controlling which function can inline
> other functions, -mpower8-fusion was set for -mcpu=power9 (-mcpu=power10 was
> not an option at that time).  This patch fixes this particular problem.
> 
> Perhaps -mpower8-fusion should go away in the GCC 13 time frame.  This patch
> just goes in and resets the fusion bit for testing inlines.
> 
> I have built a bootstrapped little endian compiler on power9 and the tests had
> no regressions.
> 
> I have built a bootstrapped big endian compiler on power8 and I tested both
> 32-bit and 64-bit builds, and there were no regressions.
> 
> Can I install this into the trunk and back port it into GCC 11 after a burn-in
> period?
> 

Thanks for the patch!  I guess we also need this for GCC 10 as:

$cat htm.c

__attribute__((always_inline)) int foo(int *b) {
  *b += 10;
  return *b;
}

#pragma GCC target "cpu=power10,htm"
int bar(int* a){
  *a = foo(a);
  return 0;
}

/opt/at14.0/bin/gcc -flto -S htm.c -mcpu=power8
htm.c:1:36: warning: ‘always_inline’ function might not be inlinable [-Wattributes]
    1 | __attribute__((always_inline)) int foo(int *b) {
      |                                    ^~~
htm.c: In function ‘bar’:
htm.c:1:36: error: inlining failed in call to ‘always_inline’ ‘foo’: target specific option mismatch
htm.c:8:8: note: called from here
    8 |   *a = foo(a);
      |        ^~~~~~

Besides, as I noted in the PR, with this fix we can safely remove the option
"-mno-power8-fusion" in gcc/testsuite/gcc.dg/lto/pr102059-1_0.c, which has
the coverage for lto (though I didn't test ;-)).

BR,
Kewen

> 2022-02-08   Michael Meissner  <meissner@linux.ibm.com>
> 
> gcc/
> 
> 	PR target/102059
> 	* config/rs6000/rs6000.cc (rs6000_can_inline_p): Don't test for
> 	power8-fusion if the caller was power9 or power10.
> 
> gcc/testsuite/
> 	PR target/102059
> 	* gcc.target/powerpc/pr102059-4.c: New test.
> ---
>  gcc/config/rs6000/rs6000.cc                   |  8 ++++++++
>  gcc/testsuite/gcc.target/powerpc/pr102059-4.c | 20 +++++++++++++++++++
>  2 files changed, 28 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr102059-4.c
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index eaba9a2d698..e2d94421ae9 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -25278,6 +25278,14 @@ rs6000_can_inline_p (tree caller, tree callee)
>  	      callee_isa &= ~OPTION_MASK_HTM;
>  	      explicit_isa &= ~OPTION_MASK_HTM;
>  	    }
> +
> +	  /* Power9 and power10 do not set power8-fusion.  If the callee was
> +	     explicitly compiled for power8, and the caller was power9 or
> +	     power10, ignore the power8-fusion bits if it was set by
> +	     default.  */
> +	  if ((caller_isa & OPTION_MASK_P8_FUSION) == 0
> +	      && (explicit_isa & OPTION_MASK_P8_FUSION) == 0)
> +	    callee_isa &= ~OPTION_MASK_P8_FUSION;
>  	}
>  
>        /* The callee's options must be a subset of the caller's options, i.e.
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr102059-4.c b/gcc/testsuite/gcc.target/powerpc/pr102059-4.c
> new file mode 100644
> index 00000000000..627c6f820c7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr102059-4.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -Wno-attributes" } */
> +
> +/* Verify that power10 can explicity include functions compiled for power8.
> +   The issue is -mcpu=power8 enables -mpower8-fusion, but -mcpu=power9 or
> +   -mcpu=power10 do not set power8-fusion by default.  */
> +
> +static inline int __attribute__ ((always_inline,target("cpu=power8")))
> +foo (int *b)
> +{
> +  *b += 10;
> +  return *b;
> +}
> +
> +int
> +bar (int *a)
> +{
> +  *a = foo (a);
> +  return 0;
> +}
>

Re: [PATCH] PR target/102059 Fix inline of target specific functions

[Michael Meissner]

On Wed, Feb 09, 2022 at 04:56:13PM +0800, Kewen.Lin wrote:
> Hi Michael,
> 
> on 2022/2/9 上午11:27, Michael Meissner via Gcc-patches wrote:
> > Reset -mpower8-fusion for power9 inlining power8 functions, PR 102059.
> > 
> > This patch is an attempt to make a much simpler patch to fix PR target/102059
> > than the previous patch.
> > 
> > It just fixes the issue that if a function is specifically declared as a power8
> > function, you can't inline in functions that are specified with power9 or
> > power10 options.
> > 
> > The issue is -mpower8-fusion is cleared when you use -mcpu=power9 or
> > -mcpu=power10.  When I wrote the code for controlling which function can inline
> > other functions, -mpower8-fusion was set for -mcpu=power9 (-mcpu=power10 was
> > not an option at that time).  This patch fixes this particular problem.
> > 
> > Perhaps -mpower8-fusion should go away in the GCC 13 time frame.  This patch
> > just goes in and resets the fusion bit for testing inlines.
> > 
> > I have built a bootstrapped little endian compiler on power9 and the tests had
> > no regressions.
> > 
> > I have built a bootstrapped big endian compiler on power8 and I tested both
> > 32-bit and 64-bit builds, and there were no regressions.
> > 
> > Can I install this into the trunk and back port it into GCC 11 after a burn-in
> > period?
> > 
> 
> Thanks for the patch!  I guess we also need this for GCC 10 as:

Gcc 10 backport is doable once it is installed in the trunk.  Since a lot of my
work is on IEEE 128-bit or Power10, I don't always think of GCC 10 backports,
but in this case, the patch is rather simple.  Thanks for the reminder.

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Ping: [PATCH] PR target/102059 Fix inline of target specific functions

[Michael Meissner]

Ping patch for PR target/102059 to ignore implicit -mpower8-fusion that
prevents a function targeting power9 or power10 from inlining a function that
declared it needed power8 via attribute/pragma target.

While we likely should revist this in GCC 13, this patch is fairly minimal in
that it fixes the specific problem Eigen ran into.  It will need to be back
ported to GCC 11 and 10 as well.

| Date: Tue, 8 Feb 2022 22:27:11 -0500
| From: Michael Meissner <meissner@linux.ibm.com>
| Subject: [PATCH] PR target/102059 Fix inline of target specific functions
| Message-ID: <YgM0jwK4niPUtP3m@toto.the-meissners.org>

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com

Re: Ping: [PATCH] PR target/102059 Fix inline of target specific functions

[Segher Boessenkool]

On Fri, Feb 11, 2022 at 12:53:07PM -0500, Michael Meissner wrote:
> Ping patch for PR target/102059 to ignore implicit -mpower8-fusion that
> prevents a function targeting power9 or power10 from inlining a function that
> declared it needed power8 via attribute/pragma target.

Can we just disable any effect from this flag, instead?  It should just
be implied by -mcpu=power8, and be impossible to be enabled otherwise
(or disabled!)


Segher

Re: Ping: [PATCH] PR target/102059 Fix inline of target specific functions

[Michael Meissner]

On Tue, Mar 08, 2022 at 11:28:03AM -0600, Segher Boessenkool wrote:
> On Fri, Feb 11, 2022 at 12:53:07PM -0500, Michael Meissner wrote:
> > Ping patch for PR target/102059 to ignore implicit -mpower8-fusion that
> > prevents a function targeting power9 or power10 from inlining a function that
> > declared it needed power8 via attribute/pragma target.
> 
> Can we just disable any effect from this flag, instead?  It should just
> be implied by -mcpu=power8, and be impossible to be enabled otherwise
> (or disabled!)

Yes, I can do that.  We should also do the same solution for power10 fusion.

What I propose is to set a regular variable with the results of the
-mpower8-fusion option.  This option would be true by default.

Then change TARGET_POWER8_FUSION to be a macro that tests whether the current
tuning CPU (not CPU we are compiling code for) to check if we are tuning for a
power8.  I would likely then remove any TARGET_POWER8 in places that test for
TARGET_POWER8_FUSION.

And similarly for TARGET_POWER10_FUSION.  Note, I haven't looked at Pat's
latest changes for power10 fusion.

Is this acceptable?

-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com