[RS6000] PR96493, powerpc local call linkage failure

[RS6000] PR96493, powerpc local call linkage failure

[Alan Modra]

This corrects current_file_function_operand, an operand predicate used
to determine whether a symbol_ref is safe to use with the local_call
patterns.  Calls between pcrel and non-pcrel code need to go via
linker stubs.  In the case of non-pcrel code to pcrel the stub saves
r2 but there needs to be a nop after the branch for the r2 restore.
So the local_call patterns can't be used there.  For pcrel code to
non-pcrel the local_call patterns could still be used, but I thought
it better to not use them since the call isn't direct.  Code generated
by the corresponding call_nonlocal_aix for pcrel is identical anyway.

Incidentally, without the TREE_CODE () == FUNCTION_DECL test,
gcc.c-torture/compile/pr37433.c and pr37433-1.c ICE.  Also, if you
make the test more strict by disallowing an op without a
SYMBOL_REF_DECL then a bunch of go and split-stack tests fail.  That's
because a prologue call to __morestack can't have a following nop.
(__morestack calls its caller at a fixed offset from the __morestack
call!)

Bootstrapped and regression tested powerpc64le-linux.  OK to apply?

Should I rename current_file_function_operand to something more
meaningful before committing?  direct_local_call_operand perhaps?

gcc/
	PR target/96493
	* config/rs6000/predicates.md (current_file_function_operand): Don't
	accept functions that differ in r2 usage.
gcc/testsuite/
	* gcc.target/powerpc/pr96493.c: New file.

diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
index afb7c02f129..2709e46f7e5 100644
--- a/gcc/config/rs6000/predicates.md
+++ b/gcc/config/rs6000/predicates.md
@@ -1051,7 +1051,12 @@
 		    && !((DEFAULT_ABI == ABI_AIX
 			  || DEFAULT_ABI == ABI_ELFv2)
 			 && (SYMBOL_REF_EXTERNAL_P (op)
-			     || SYMBOL_REF_WEAK (op)))")))
+			     || SYMBOL_REF_WEAK (op)))
+		    && !(DEFAULT_ABI == ABI_ELFv2
+			 && SYMBOL_REF_DECL (op) != NULL
+			 && TREE_CODE (SYMBOL_REF_DECL (op)) == FUNCTION_DECL
+			 && (rs6000_fndecl_pcrel_p (SYMBOL_REF_DECL (op))
+			     != rs6000_pcrel_p (cfun)))")))
 
 ;; Return 1 if this operand is a valid input for a move insn.
 (define_predicate "input_operand"
diff --git a/gcc/testsuite/gcc.target/powerpc/pr96493.c b/gcc/testsuite/gcc.target/powerpc/pr96493.c
new file mode 100644
index 00000000000..3ee0fc9fe45
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr96493.c
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+/* { dg-options "-mdejagnu-cpu=powerpc64 -O2" } */
+/* { dg-require-effective-target powerpc_elfv2 } */
+
+/* Test local calls between pcrel and non-pcrel code.
+
+   Despite the cpu=power10 option, the code generated here should just
+   be plain powerpc64, even the necessary linker stubs.  */
+
+int one = 1;
+
+int __attribute__ ((target("cpu=power8"),noclone,noinline))
+p8_func (int x)
+{
+  return x - one;
+}
+
+int __attribute__ ((target("cpu=power10"),noclone,noinline))
+p10_func (int x)
+{
+  return p8_func (x);
+}
+
+int
+main (void)
+{
+  return p10_func (1);
+}

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] PR96493, powerpc local call linkage failure

[Segher Boessenkool]

Hi!

On Thu, Aug 06, 2020 at 10:58:18PM +0930, Alan Modra wrote:
> This corrects current_file_function_operand, an operand predicate used
> to determine whether a symbol_ref is safe to use with the local_call
> patterns.  Calls between pcrel and non-pcrel code need to go via
> linker stubs.  In the case of non-pcrel code to pcrel the stub saves
> r2 but there needs to be a nop after the branch for the r2 restore.
> So the local_call patterns can't be used there.

Okay.

> For pcrel code to
> non-pcrel the local_call patterns could still be used, but I thought
> it better to not use them since the call isn't direct.  Code generated
> by the corresponding call_nonlocal_aix for pcrel is identical anyway.

Hrm.

> Should I rename current_file_function_operand to something more
> meaningful before committing?  direct_local_call_operand perhaps?

As a separate patch, either before or after this one.  And maybe a
better name than that as well, direct_local_call_operand isn't great?

In the same vein, maybe the local_call pattern names should be changed?
Because this isn't used just for local calls anymore; instead, the
defining characteristic is whether there is a restore of r2 after the
call (whether there might be any such restore needed).  The pattern
names and the operand name ideally would be obviously related?

> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -1051,7 +1051,12 @@
>  		    && !((DEFAULT_ABI == ABI_AIX
>  			  || DEFAULT_ABI == ABI_ELFv2)
>  			 && (SYMBOL_REF_EXTERNAL_P (op)
> -			     || SYMBOL_REF_WEAK (op)))")))
> +			     || SYMBOL_REF_WEAK (op)))
> +		    && !(DEFAULT_ABI == ABI_ELFv2
> +			 && SYMBOL_REF_DECL (op) != NULL
> +			 && TREE_CODE (SYMBOL_REF_DECL (op)) == FUNCTION_DECL
> +			 && (rs6000_fndecl_pcrel_p (SYMBOL_REF_DECL (op))
> +			     != rs6000_pcrel_p (cfun)))")))

This condition is much too complex like that...  can you factor it out
to a code block, perhaps?  Or maybe there should be an actual helper
function.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr96493.c
> @@ -0,0 +1,28 @@
> +/* { dg-do run } */
> +/* { dg-options "-mdejagnu-cpu=powerpc64 -O2" } */

That is not a -mcpu= value you should ever use.  Please just pick a real
existing CPU, maybe p7 or p8 since this requires ELFv2 anyway?  Or, what
does it need here?  It isn't clear to me.  But you don't want a pseudo-
POWER3 with ELFv2 :-)

The patch is okay for trunk (and backports later) if you fix the
testcase (the renames and other improvements can be done later, and do
not need backporting).  Thanks!


Segher

Re: [RS6000] PR96493, powerpc local call linkage failure

[Alan Modra]

On Thu, Aug 06, 2020 at 05:34:03PM -0500, Segher Boessenkool wrote:
> > +/* { dg-do run } */
> > +/* { dg-options "-mdejagnu-cpu=powerpc64 -O2" } */
> 
> That is not a -mcpu= value you should ever use.  Please just pick a real
> existing CPU, maybe p7 or p8 since this requires ELFv2 anyway?  Or, what
> does it need here?  It isn't clear to me.  But you don't want a pseudo-
> POWER3 with ELFv2 :-)

What I was trying to say by using cpu=powerpc64 is that this test has
minimal requirements on the required cpu level to run the test.
That's all.  Changed to power8 and committed.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] PR96493, powerpc local call linkage failure

[Segher Boessenkool]

On Fri, Aug 07, 2020 at 12:58:08PM +0930, Alan Modra wrote:
> On Thu, Aug 06, 2020 at 05:34:03PM -0500, Segher Boessenkool wrote:
> > > +/* { dg-do run } */
> > > +/* { dg-options "-mdejagnu-cpu=powerpc64 -O2" } */
> > 
> > That is not a -mcpu= value you should ever use.  Please just pick a real
> > existing CPU, maybe p7 or p8 since this requires ELFv2 anyway?  Or, what
> > does it need here?  It isn't clear to me.  But you don't want a pseudo-
> > POWER3 with ELFv2 :-)
> 
> What I was trying to say by using cpu=powerpc64 is that this test has
> minimal requirements on the required cpu level to run the test.
> That's all.

That would be -mcpu=powerpc64le, which is the exact same as -mcpu=power8.
-mcpu=powerpc64 is something close to 620/630 (power3, but it is not
exactly the same as -mcpu=power3) :-)

> Changed to power8 and committed.

Thanks!


Segher

Re: [RS6000] PR96493, powerpc local call linkage failure

[Alan Modra]

This fixes a fail when power10 isn't supported by binutils, and
ensures the test isn't run without power10 hardware or simulation on
the off chance that power10 insns are emitted in the future for this
testcase.  Bootstrapped etc.  OK?

	PR target/96525
	* testsuite/gcc.target/powerpc/pr96493.c: Make it a link test
	when no power10_hw.  Require power10_ok.

diff --git a/gcc/testsuite/gcc.target/powerpc/pr96493.c b/gcc/testsuite/gcc.target/powerpc/pr96493.c
index f0de0818813..1e5d43f199d 100644
--- a/gcc/testsuite/gcc.target/powerpc/pr96493.c
+++ b/gcc/testsuite/gcc.target/powerpc/pr96493.c
@@ -1,6 +1,8 @@
-/* { dg-do run } */
+/* { dg-do run { target { power10_hw } } } */
+/* { dg-do link { target { ! power10_hw } } } */
 /* { dg-options "-mdejagnu-cpu=power8 -O2" } */
 /* { dg-require-effective-target powerpc_elfv2 } */
+/* { dg-require-effective-target power10_ok } */
 
 /* Test local calls between pcrel and non-pcrel code.
 

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RS6000] PR96493, powerpc local call linkage failure

[Segher Boessenkool]

Hi Alan,

On Tue, Aug 11, 2020 at 06:38:53PM +0930, Alan Modra wrote:
> This fixes a fail when power10 isn't supported by binutils, and
> ensures the test isn't run without power10 hardware or simulation on
> the off chance that power10 insns are emitted in the future for this
> testcase.

The testcases said it wanted power8, so why did it fail?  GCC shouldn't
use anything that requires p10 support in binutils then, or what do I
miss here?


Segher


> --- a/gcc/testsuite/gcc.target/powerpc/pr96493.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr96493.c
> @@ -1,6 +1,8 @@
> -/* { dg-do run } */
> +/* { dg-do run { target { power10_hw } } } */
> +/* { dg-do link { target { ! power10_hw } } } */
>  /* { dg-options "-mdejagnu-cpu=power8 -O2" } */
>  /* { dg-require-effective-target powerpc_elfv2 } */
> +/* { dg-require-effective-target power10_ok } */
>  
>  /* Test local calls between pcrel and non-pcrel code.
>  

Re: [RS6000] PR96493, powerpc local call linkage failure

[Peter Bergner]

On 8/11/20 11:35 AM, Segher Boessenkool wrote:
> Hi Alan,
> 
> On Tue, Aug 11, 2020 at 06:38:53PM +0930, Alan Modra wrote:
>> This fixes a fail when power10 isn't supported by binutils, and
>> ensures the test isn't run without power10 hardware or simulation on
>> the off chance that power10 insns are emitted in the future for this
>> testcase.
> 
> The testcases said it wanted power8, so why did it fail?  GCC shouldn't
> use anything that requires p10 support in binutils then, or what do I
> miss here?

It failed with an assembler error because one of the functions in the
test uses an attribute target power10 and GCC emits a ".machine power10"
assembler directive in case we do generate a power10 instruction(s).
The old binutils Bill used doesn't know about power10, so boom.
That is what requires the dg-require-effective-target power10_ok.

Now given the power10 function is so small (just a call to a p8
function), the chance we'll generate a p10 instruction is low (zero?),
so we could just keep the dg-do run as is (ie, always run), but
might that change one day?

Peter

Re: [RS6000] PR96493, powerpc local call linkage failure

[Segher Boessenkool]

On Tue, Aug 11, 2020 at 12:36:28PM -0500, Peter Bergner wrote:
> On 8/11/20 11:35 AM, Segher Boessenkool wrote:
> > Hi Alan,
> > 
> > On Tue, Aug 11, 2020 at 06:38:53PM +0930, Alan Modra wrote:
> >> This fixes a fail when power10 isn't supported by binutils, and
> >> ensures the test isn't run without power10 hardware or simulation on
> >> the off chance that power10 insns are emitted in the future for this
> >> testcase.
> > 
> > The testcases said it wanted power8, so why did it fail?  GCC shouldn't
> > use anything that requires p10 support in binutils then, or what do I
> > miss here?
> 
> It failed with an assembler error because one of the functions in the
> test uses an attribute target power10 and GCC emits a ".machine power10"
> assembler directive in case we do generate a power10 instruction(s).
> The old binutils Bill used doesn't know about power10, so boom.
> That is what requires the dg-require-effective-target power10_ok.

Ah, okay.

> Now given the power10 function is so small (just a call to a p8
> function), the chance we'll generate a p10 instruction is low (zero?),
> so we could just keep the dg-do run as is (ie, always run), but
> might that change one day?

On a non-p10 it will just use the generated non-p10 code, and that will
just work, now and for forever (yeah right :-) )

Either always running or what this patch does will work.  But please add
comments what the test case wants to test, and for the tricky bits.


Segher

Re: [RS6000] PR96493, powerpc local call linkage failure

[Alan Modra]

On Tue, Aug 11, 2020 at 01:30:36PM -0500, Segher Boessenkool wrote:
> Either always running or what this patch does will work.  But please add
> comments what the test case wants to test,

That's already in the testcase.

	/* Test local calls between pcrel and non-pcrel code.

The comment goes on to say

	   Despite the cpu=power10 option, the code generated here should just
	   be plain powerpc64, even the necessary linker stubs.  */

which was the justification for using "dg-do run" unqualified in the
current testcase.

> and for the tricky bits.

There aren't any.  And other tests use multiple dg-do lines, eg.
gcc/testsuite/g++.dg/ext/altivec-3.C

/* { dg-do run { target { powerpc*-*-* && vmx_hw } } } */
/* { dg-do compile { target { powerpc*-*-* && { ! vmx_hw } } } } */

Committed 2ba0674c657, and apologies for missing the power10_ok first
time around on this test.

-- 
Alan Modra
Australia Development Lab, IBM