Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check
Alan Modra writes: > On Fri, Oct 30, 2020 at 09:21:09AM +, Richard Sandiford wrote: >> Alan Modra via Gcc-patches 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] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check
On Fri, Oct 30, 2020 at 09:21:09AM +, Richard Sandiford wrote: > Alan Modra via Gcc-patches 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
Re: [PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check
Alan Modra via Gcc-patches 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
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
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
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
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
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
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
[PATCH] calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check
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))
+{
+
