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
