[1/2] can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check V2

On Sat, Oct 31, 2020 at 12:10:35AM +1030, Alan Modra wrote:
> Would it be better if I post the patches again, restructuring them as
> 1) completely no functional change just moving the existing condition
>    to the powerpc and i386 target hooks, and
> 2) twiddling the powerpc target hook?

The no function change patch.

This moves an #ifdef block of code from calls.c to
targetm.function_ok_for_sibcall.  Only two targets, x86 and rs6000,
define REG_PARM_STACK_SPACE or OUTGOING_REG_PARM_STACK_SPACE macros
that might vary depending on the called function.  Macros like
UNITS_PER_WORD don't change over a function boundary, nor does the
MIPS ABI, nor does TARGET_64BIT on PA-RISC.  Other targets are even
more trivially proven to not need the calls.c code.

Besides cleaning up a small piece of #ifdef code, the motivation for
this patch is to allow tail calls on PowerPC for functions that
require less reg_parm_stack_space than their caller.  The original
code in calls.c only permitted tail calls when exactly equal.  That
will be done in a followup patch (removing the code added to
rs6000-logue.c in this patch).

Bootstrapped and regression tested powerpc64le-linux, powerpc64-linux,
and x86_64-linux.  OK?

	PR middle-end/97267
	* calls.h (maybe_complain_about_tail_call): Declare.
	* calls.c (maybe_complain_about_tail_call): Make global.
	(can_implement_as_sibling_call_p): Delete reg_parm_stack_space
	param.  Adjust caller.  Move REG_PARM_STACK_SPACE check to..
	* config/i386/i386.c (ix86_function_ok_for_sibcall): ..here, and..
	* config/rs6000/rs6000-logue.c (rs6000_function_ok_for_sibcall): ..
	here.

Eh, never mind the previous message, I managed to miss the follow-ups.

Alan Modra <amodra@gmail.com> writes:
> On Sat, Oct 31, 2020 at 12:10:35AM +1030, Alan Modra wrote:
>> Would it be better if I post the patches again, restructuring them as
>> 1) completely no functional change just moving the existing condition
>>    to the powerpc and i386 target hooks, and
>> 2) twiddling the powerpc target hook?
>
> The no function change patch.
>
> This moves an #ifdef block of code from calls.c to
> targetm.function_ok_for_sibcall.  Only two targets, x86 and rs6000,
> define REG_PARM_STACK_SPACE or OUTGOING_REG_PARM_STACK_SPACE macros
> that might vary depending on the called function.  Macros like
> UNITS_PER_WORD don't change over a function boundary, nor does the
> MIPS ABI, nor does TARGET_64BIT on PA-RISC.  Other targets are even
> more trivially proven to not need the calls.c code.
>
> Besides cleaning up a small piece of #ifdef code, the motivation for
> this patch is to allow tail calls on PowerPC for functions that
> require less reg_parm_stack_space than their caller.  The original
> code in calls.c only permitted tail calls when exactly equal.  That
> will be done in a followup patch (removing the code added to
> rs6000-logue.c in this patch).
>
> Bootstrapped and regression tested powerpc64le-linux, powerpc64-linux,
> and x86_64-linux.  OK?

OK provided that the rs6000 part is OK.

I think it makes more sense to keep and commit this as a combined patch
with the rs6000 part, rather than as a stand-alone change.  I think it's
hard to justify doing this while both the affected targets continue to
check something related to the old condition.  But it's easy to justify
when the condition is being removed entirely from one target.

Thanks,
Richard

>
> 	PR middle-end/97267
> 	* calls.h (maybe_complain_about_tail_call): Declare.
> 	* calls.c (maybe_complain_about_tail_call): Make global.
> 	(can_implement_as_sibling_call_p): Delete reg_parm_stack_space
> 	param.  Adjust caller.  Move REG_PARM_STACK_SPACE check to..
> 	* config/i386/i386.c (ix86_function_ok_for_sibcall): ..here, and..
> 	* config/rs6000/rs6000-logue.c (rs6000_function_ok_for_sibcall): ..
> 	here.
>
> diff --git a/gcc/calls.c b/gcc/calls.c
> index a8f459632f2..1a7632d2d48 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -1922,7 +1922,7 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
>  /* Issue an error if CALL_EXPR was flagged as requiring
>     tall-call optimization.  */
>  
> -static void
> +void
>  maybe_complain_about_tail_call (tree call_expr, const char *reason)
>  {
>    gcc_assert (TREE_CODE (call_expr) == CALL_EXPR);
> @@ -3525,7 +3525,6 @@ static bool
>  can_implement_as_sibling_call_p (tree exp,
>  				 rtx structure_value_addr,
>  				 tree funtype,
> -				 int reg_parm_stack_space ATTRIBUTE_UNUSED,
>  				 tree fndecl,
>  				 int flags,
>  				 tree addr,
> @@ -3550,20 +3549,6 @@ can_implement_as_sibling_call_p (tree exp,
>        return false;
>      }
>  
> -#ifdef REG_PARM_STACK_SPACE
> -  /* If outgoing reg parm stack space changes, we cannot do sibcall.  */
> -  if (OUTGOING_REG_PARM_STACK_SPACE (funtype)
> -      != OUTGOING_REG_PARM_STACK_SPACE (TREE_TYPE (current_function_decl))
> -      || (reg_parm_stack_space != REG_PARM_STACK_SPACE (current_function_decl)))
> -    {
> -      maybe_complain_about_tail_call (exp,
> -				      "inconsistent size of stack space"
> -				      " allocated for arguments which are"
> -				      " passed in registers");
> -      return false;
> -    }
> -#endif
> -
>    /* Check whether the target is able to optimize the call
>       into a sibcall.  */
>    if (!targetm.function_ok_for_sibcall (fndecl, exp))
> @@ -4088,7 +4073,6 @@ expand_call (tree exp, rtx target, int ignore)
>      try_tail_call = can_implement_as_sibling_call_p (exp,
>  						     structure_value_addr,
>  						     funtype,
> -						     reg_parm_stack_space,
>  						     fndecl,
>  						     flags, addr, args_size);
>  
> diff --git a/gcc/calls.h b/gcc/calls.h
> index f32b6308b58..b20d24bb888 100644
> --- a/gcc/calls.h
> +++ b/gcc/calls.h
> @@ -133,6 +133,7 @@ extern bool reference_callee_copied (CUMULATIVE_ARGS *,
>  extern void maybe_warn_alloc_args_overflow (tree, tree, tree[2], int[2]);
>  extern tree get_attr_nonstring_decl (tree, tree * = NULL);
>  extern bool maybe_warn_nonstring_arg (tree, tree);
> +extern void maybe_complain_about_tail_call (tree, const char *);
>  enum size_range_flags
>    {
>     /* Set to consider zero a valid range.  */
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 502d24057b5..809c145b638 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -939,6 +939,19 @@ ix86_function_ok_for_sibcall (tree decl, tree exp)
>        decl_or_type = type;
>      }
>  
> +  /* If outgoing reg parm stack space changes, we cannot do sibcall.  */
> +  if ((OUTGOING_REG_PARM_STACK_SPACE (type)
> +       != OUTGOING_REG_PARM_STACK_SPACE (TREE_TYPE (current_function_decl)))
> +      || (REG_PARM_STACK_SPACE (decl_or_type)
> +	  != REG_PARM_STACK_SPACE (current_function_decl)))
> +    {
> +      maybe_complain_about_tail_call (exp,
> +				      "inconsistent size of stack space"
> +				      " allocated for arguments which are"
> +				      " passed in registers");
> +      return false;
> +    }
> +
>    /* Check that the return value locations are the same.  Like
>       if we are returning floats on the 80387 register stack, we cannot
>       make a sibcall from a function that doesn't return a float to a
> diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
> index d90cd5736e1..61eb7ce7ade 100644
> --- a/gcc/config/rs6000/rs6000-logue.c
> +++ b/gcc/config/rs6000/rs6000-logue.c
> @@ -30,6 +30,7 @@
>  #include "df.h"
>  #include "tm_p.h"
>  #include "ira.h"
> +#include "calls.h"
>  #include "print-tree.h"
>  #include "varasm.h"
>  #include "explow.h"
> @@ -1133,6 +1134,19 @@ rs6000_function_ok_for_sibcall (tree decl, tree exp)
>    else
>      fntype = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
>  
> +  /* If outgoing reg parm stack space changes, we cannot do sibcall.  */
> +  if ((OUTGOING_REG_PARM_STACK_SPACE (fntype)
> +       != OUTGOING_REG_PARM_STACK_SPACE (TREE_TYPE (current_function_decl)))
> +      || (REG_PARM_STACK_SPACE (decl ? decl : fntype)
> +	  != REG_PARM_STACK_SPACE (current_function_decl)))
> +    {
> +      maybe_complain_about_tail_call (exp,
> +				      "inconsistent size of stack space"
> +				      " allocated for arguments which are"
> +				      " passed in registers");
> +      return false;
> +    }
> +
>    /* We can't do it if the called function has more vector parameters
>       than the current function; there's nowhere to put the VRsave code.  */
>    if (TARGET_ALTIVEC_ABI

Hi!

On Mon, Nov 02, 2020 at 08:16:16PM +1030, Alan Modra wrote:
> The no function change patch.
> 
> This moves an #ifdef block of code from calls.c to
> targetm.function_ok_for_sibcall.  Only two targets, x86 and rs6000,
> define REG_PARM_STACK_SPACE or OUTGOING_REG_PARM_STACK_SPACE macros
> that might vary depending on the called function.  Macros like
> UNITS_PER_WORD don't change over a function boundary, nor does the
> MIPS ABI, nor does TARGET_64BIT on PA-RISC.  Other targets are even
> more trivially proven to not need the calls.c code.

Just the rs6000 part:

> +  /* If outgoing reg parm stack space changes, we cannot do sibcall.  */
> +  if ((OUTGOING_REG_PARM_STACK_SPACE (fntype)
> +       != OUTGOING_REG_PARM_STACK_SPACE (TREE_TYPE (current_function_decl)))
> +      || (REG_PARM_STACK_SPACE (decl ? decl : fntype)
> +	  != REG_PARM_STACK_SPACE (current_function_decl)))

Please don't use superfluous parens, like (a) || (b) here, it makes
things harder to read than necessary.

Other than that this patch is fine.  Thanks!


Segher