[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