Message ID | 20200806132818.GF15695@bubble.grove.modra.org |
---|---|
State | New |
Headers | show |
Series | [RS6000] PR96493, powerpc local call linkage failure | expand |
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
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.
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
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.
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. >
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
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
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.
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); +}