[PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Alan Modra]

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 seen 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.

Bootstrapped and regression tested powerpc64le-linux and
x86_64-linux.  OK for master?

	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): 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.  Modify to allow reg_parm_stack_space less or equal to
	caller, and delete redundant OUTGOING_REG_PARM_STACK_SPACE test.
	* testsuite/gcc.target/powerpc/pr97267.c: New test.

diff --git a/gcc/calls.c b/gcc/calls.c
index ed4363811c8..df7324f9343 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1873,7 +1873,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);
@@ -3501,20 +3501,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))
diff --git a/gcc/calls.h b/gcc/calls.h
index dfb951ca45b..6d4feb59dd0 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 *);
 extern bool get_size_range (tree, tree[2], bool = false);
 extern rtx rtx_for_static_chain (const_tree, bool);
 extern bool cxx17_empty_base_field_p (const_tree);
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index f684954af81..58fc5280935 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..814b549e4ca 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,17 @@ rs6000_function_ok_for_sibcall (tree decl, tree exp)
   else
     fntype = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
 
+  /* If reg parm stack space increases, we cannot sibcall.  */
+  if (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
diff --git a/gcc/testsuite/gcc.target/powerpc/pr97267.c b/gcc/testsuite/gcc.target/powerpc/pr97267.c
new file mode 100644
index 00000000000..cab46245fc9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr97267.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+static int __attribute__ ((__noclone__, __noinline__))
+reg_args (int j1, int j2, int j3, int j4, int j5, int j6, int j7, int j8)
+{
+  return j1 + j2 + j3 + j4 + j5 + j6 + j7 + j8;
+}
+
+int __attribute__ ((__noclone__, __noinline__))
+stack_args (int j1, int j2, int j3, int j4, int j5, int j6, int j7, int j8,
+	    int j9)
+{
+  if (j9 == 0)
+    return 0;
+  return reg_args (j1, j2, j3, j4, j5, j6, j7, j8);
+}
+
+/* { dg-final { scan-assembler-not {(?n)^\s+bl\s} } } */

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Alan Modra]

On Fri, Oct 02, 2020 at 04:41:05PM +0930, Alan Modra wrote:
> This moves an #ifdef block of code from calls.c to
> targetm.function_ok_for_sibcall.

Not only did I miss cc'ing the i386 maintainers, I missed seeing that
the ATTRIBUTE_UNUSED reg_parm_stack_space param of
can_implement_as_sibling_call_p is now always unused.  Please consider
that parameter removed.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Segher Boessenkool]

Hi!

On Fri, Oct 02, 2020 at 04:41:05PM +0930, Alan Modra wrote:
> 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 seen 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.

> +  /* If reg parm stack space increases, we cannot sibcall.  */
> +  if (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;
> +    }

Maybe change the message?  You allow all sizes smaller or equal than
the current size, "inconsistent" isn't very great for that.

> +static int __attribute__ ((__noclone__, __noinline__))
> +reg_args (int j1, int j2, int j3, int j4, int j5, int j6, int j7, int j8)
> +{
> +  return j1 + j2 + j3 + j4 + j5 + j6 + j7 + j8;
> +}
> +
> +int __attribute__ ((__noclone__, __noinline__))
> +stack_args (int j1, int j2, int j3, int j4, int j5, int j6, int j7, int j8,
> +	    int j9)
> +{
> +  if (j9 == 0)
> +    return 0;
> +  return reg_args (j1, j2, j3, j4, j5, j6, j7, j8);
> +}
> +
> +/* { dg-final { scan-assembler-not {(?n)^\s+bl\s} } } */

Nice testcase :-)  (You can do \M or even just a space instead of that
last \s, but this works fine.)

The rs6000 parts are fine (with the message improved perhaps).  Thanks!
The rest looks fine to me as well, fwiw.


Segher

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Alan Modra]

Hi Segher,

On Fri, Oct 02, 2020 at 01:50:24PM -0500, Segher Boessenkool wrote:
> On Fri, Oct 02, 2020 at 04:41:05PM +0930, Alan Modra wrote:
> > 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 seen 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.
> 
> > +  /* If reg parm stack space increases, we cannot sibcall.  */
> > +  if (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;
> > +    }
> 
> Maybe change the message?  You allow all sizes smaller or equal than
> the current size, "inconsistent" isn't very great for that.

We're talking about just two sizes here.  For 64-bit ELFv2 the reg
parm save size is either 0 or 64 bytes.  Yes, a better message would
be "caller lacks stack space allocated for aguments passed in
registers, required by callee".

Note that I'll likely be submitting a further patch that removes the
above code in rs6000-logue.c.  I thought is safer to only make a small
change at the same time as moving code around.

The reasoning behind a followup patch is:
a) The generic code checks that arg passing space in the called
   function is not greater than that in the current function, and,
b) ELFv2 only allocates reg_parm_stack_space when some parameter is
   passed on the stack.
Point (b) means that zero reg_parm_stack_space implies zero stack
arg space, and non-zero reg_parm_stack_space implies non-zero stack
arg space.  So the case of 0 reg_parm_stack_space in the caller and 64
in the callee will be caught by (a).

Also, there's a bug in the code I moved from calls.c.  It should be
using INCOMING_REG_PARM_STACK_SPACE, to properly compare space known
to be allocated for the current function vs. space needed for the
called function.  For an explanation of INCOMING_REG_PARM_STACK_SPACE
see https://gcc.gnu.org/pipermail/gcc-patches/2014-May/389867.html
Of course that bug doesn't matter in this context because it's always
been covered up by (a).

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Segher Boessenkool]

Hi!

On Sun, Oct 04, 2020 at 11:09:11PM +1030, Alan Modra wrote:
> On Fri, Oct 02, 2020 at 01:50:24PM -0500, Segher Boessenkool wrote:
> > > +  /* If reg parm stack space increases, we cannot sibcall.  */
> > > +  if (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;
> > > +    }
> > 
> > Maybe change the message?  You allow all sizes smaller or equal than
> > the current size, "inconsistent" isn't very great for that.
> 
> We're talking about just two sizes here.  For 64-bit ELFv2 the reg
> parm save size is either 0 or 64 bytes.  Yes, a better message would
> be "caller lacks stack space allocated for aguments passed in
> registers, required by callee".

Something like that yes.  However:

> Note that I'll likely be submitting a further patch that removes the
> above code in rs6000-logue.c.  I thought is safer to only make a small
> change at the same time as moving code around.

Yeah, just keep it then.


Segher

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Alan Modra]

Ping?

On Fri, Oct 02, 2020 at 05:03:50PM +0930, Alan Modra wrote:
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555309.html

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Alan Modra]

On Mon, Oct 12, 2020 at 10:37:05PM +1030, Alan Modra wrote:
> Ping?
> 
> On Fri, Oct 02, 2020 at 05:03:50PM +0930, Alan Modra wrote:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555309.html

Ping^2

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Richard Sandiford]

Alan Modra via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> 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 seen 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.

Is there something PowerPC-specific that makes the relaxation safe
for that target while not being safe on x86?

I take your point about x86 and PowerPC being the only two affected
targets.  But the interface does still take an fndecl on all targets,
so I think the target-independent assumption should be that the value
might vary depending on function.  So I guess an alternative would be
to relax the target-independent condition and make the x86 hook enforce
the stricter condition (if it really is needed).

Thanks,
Richard

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Alan Modra]

On Fri, Oct 30, 2020 at 09:21:09AM +0000, Richard Sandiford wrote:
> Alan Modra via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > 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 seen 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.
> 
> Is there something PowerPC-specific that makes the relaxation safe
> for that target while not being safe on x86?

It is quite possible that x86 can relax this condition too, I'm just
not familiar enough with all the x86 ABIs know with any certainty.  By
moving the test to the target hook we allow target maintainers to have
full say in the matter.

> I take your point about x86 and PowerPC being the only two affected
> targets.  But the interface does still take an fndecl on all targets,
> so I think the target-independent assumption should be that the value
> might vary depending on function.  So I guess an alternative would be
> to relax the target-independent condition and make the x86 hook enforce
> the stricter condition (if it really is needed).

Yes, except that actually the REG_PARM_STACK_SPACE condition for
PowerPC can be removed entirely.  I agree that doing as you suggest
would be OK for PowerPC, it would just mean we continue to do some
unnecessary work in the non-trivial rs6000_function_parms_need_stack.

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?

Thanks for your time spent reviewing, and comments!

-- 
Alan Modra
Australia Development Lab, IBM

[PATCH 1/2] can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check V2

[Alan Modra]

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.

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

-- 
Alan Modra
Australia Development Lab, IBM

[PATCH 2/2] [RS6000] REG_PARM_STACK_SPACE check V2

[Alan Modra]

On PowerPC we can tail call if the callee has less or equal
REG_PARM_STACK_SPACE than the caller, as demonstrated by the
testcase.  So we should use

  /* If reg parm stack space increases, we cannot sibcall.  */
  if (REG_PARM_STACK_SPACE (decl ? decl : fntype)
      > INCOMING_REG_PARM_STACK_SPACE (current_function_decl))

and note the change to use INCOMING_REG_PARM_STACK_SPACE.
REG_PARM_STACK_SPACE has always been wrong there for PowerPC.  See
https://gcc.gnu.org/pipermail/gcc-patches/2014-May/389867.html for why
if you're curious.  Not that it matters, because PowerPC can do
without this check entirely, relying on a stack slot test in generic
code.

a) The generic code checks that arg passing stack in the callee is not
   greater than that in the caller, and,
b) ELFv2 only allocates reg_parm_stack_space when some parameter is
   passed on the stack.
Point (b) means that zero reg_parm_stack_space implies zero stack
space, and non-zero reg_parm_stack_space implies non-zero stack
space.  So the case of 0 reg_parm_stack_space in the caller and 64 in
the callee will be caught by (a).

Bootstrapped and regression tested powerpc64le-linux and biarch
powerpc64-linux. OK?

	PR middle-end/97267
gcc/
	* config/rs6000/rs6000-logue.c (rs6000_function_ok_for_sibcall):
	Remove code checking REG_PARM_STACK_SPACE.
testsuite/
	* gcc.target/powerpc/pr97267.c: New test.

diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
index 61eb7ce7ade..d90cd5736e1 100644
--- a/gcc/config/rs6000/rs6000-logue.c
+++ b/gcc/config/rs6000/rs6000-logue.c
@@ -30,7 +30,6 @@
 #include "df.h"
 #include "tm_p.h"
 #include "ira.h"
-#include "calls.h"
 #include "print-tree.h"
 #include "varasm.h"
 #include "explow.h"
@@ -1134,19 +1133,6 @@ 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
diff --git a/gcc/testsuite/gcc.target/powerpc/pr97267.c b/gcc/testsuite/gcc.target/powerpc/pr97267.c
new file mode 100644
index 00000000000..cab46245fc9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr97267.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+static int __attribute__ ((__noclone__, __noinline__))
+reg_args (int j1, int j2, int j3, int j4, int j5, int j6, int j7, int j8)
+{
+  return j1 + j2 + j3 + j4 + j5 + j6 + j7 + j8;
+}
+
+int __attribute__ ((__noclone__, __noinline__))
+stack_args (int j1, int j2, int j3, int j4, int j5, int j6, int j7, int j8,
+	    int j9)
+{
+  if (j9 == 0)
+    return 0;
+  return reg_args (j1, j2, j3, j4, j5, j6, j7, j8);
+}
+
+/* { dg-final { scan-assembler-not {(?n)^\s+bl\s} } } */

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check

[Richard Sandiford]

Alan Modra <amodra@gmail.com> writes:
> On Fri, Oct 30, 2020 at 09:21:09AM +0000, Richard Sandiford wrote:
>> Alan Modra via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> > 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 seen 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.
>> 
>> Is there something PowerPC-specific that makes the relaxation safe
>> for that target while not being safe on x86?
>
> It is quite possible that x86 can relax this condition too, I'm just
> not familiar enough with all the x86 ABIs know with any certainty.  By
> moving the test to the target hook we allow target maintainers to have
> full say in the matter.
>
>> I take your point about x86 and PowerPC being the only two affected
>> targets.  But the interface does still take an fndecl on all targets,
>> so I think the target-independent assumption should be that the value
>> might vary depending on function.  So I guess an alternative would be
>> to relax the target-independent condition and make the x86 hook enforce
>> the stricter condition (if it really is needed).
>
> Yes, except that actually the REG_PARM_STACK_SPACE condition for
> PowerPC can be removed entirely.  I agree that doing as you suggest
> would be OK for PowerPC, it would just mean we continue to do some
> unnecessary work in the non-trivial rs6000_function_parms_need_stack.

Ah, OK.  If you did the check in rs6000_function_parms_need_stack, what
would the final version of the condition look like, including the future
changes you have planned?

My main point here is that I think it would be good to have
target-independent code check the lowest common denominator that we
know GCC can support on targets with nonzero REG_PARM_STACK_SPACEs,
rather than force all those targets to repeat the check.  (Admittedly
“all” is just “two” as things stand, but still…)

Targets like i386 can conservatively continue to enforce the old
condition but target-independent code would follow the new and more
relaxed rs6000 rules.

Thanks,
Richard

Re: [PATCH 1/2] can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check V2

[Richard Sandiford]

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

Re: [PATCH 1/2] can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check V2

[Segher Boessenkool]

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

Re: [PATCH 2/2] [RS6000] REG_PARM_STACK_SPACE check V2

[Segher Boessenkool]

On Mon, Nov 02, 2020 at 08:19:08PM +1030, Alan Modra wrote:
> On PowerPC we can tail call if the callee has less or equal
> REG_PARM_STACK_SPACE than the caller, as demonstrated by the
> testcase.  So we should use
> 
>   /* If reg parm stack space increases, we cannot sibcall.  */
>   if (REG_PARM_STACK_SPACE (decl ? decl : fntype)
>       > INCOMING_REG_PARM_STACK_SPACE (current_function_decl))
> 
> and note the change to use INCOMING_REG_PARM_STACK_SPACE.

> Bootstrapped and regression tested powerpc64le-linux and biarch
> powerpc64-linux. OK?

Yes please.  Thanks!


Segher


> 	PR middle-end/97267
> gcc/
> 	* config/rs6000/rs6000-logue.c (rs6000_function_ok_for_sibcall):
> 	Remove code checking REG_PARM_STACK_SPACE.
> testsuite/
> 	* gcc.target/powerpc/pr97267.c: New test.