Message ID | 20201002071105.GP15011@bubble.grove.modra.org |
---|---|
State | New |
Headers | show |
Series | calls.c:can_implement_as_sibling_call_p REG_PARM_STACK_SPACE check | expand |
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.
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
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).
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
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
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 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
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 <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
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} } } */