This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][expr.c] PR 65358 Avoid clobbering partial argument during sibcall


On 05/11/2015 03:28 AM, Kyrill Tkachov wrote:
The more I think about this, the more I think it's an ugly can of
worms and maybe we should just disable sibcalls for partial
arguments.  I doubt it's a big performance issue in general.

We already have quite a bit of code in calls.c to detect cases with
partial argument overlap for the
explicit purpose of allowing sibcalls when partial arguments occur in
the general case. However, that
code only detects when a partial argument overlaps with other arguments
in a call. In this PR the
partial argument overlaps with itself. It would be a shame to disable
sibcalls for all partial arguments
when there is already infrastructure in place to handle them.
I didn't even realize we had support for partial arguments in sibcalls. Ah, Kazu added that in 2005, I totally missed it. I probably would have suggested failing the sibcall for those cases back then too...

Is there any way to re-use that infrastructure to deal with the case at hand?






In addition to the argument/stack direction stuff, I've been pondering
the stack/frame/arg pointer issues.  Your approach assumes that the
incoming and outgoing areas are always referenced off the same base
register.  If they aren't, then the routine returns no overlap.

But we'd need to consider the case where we have a reference to the
arg or frame pointer which later gets rewritten into a stack pointer
relative address.

Is it too late at the point were you do the checks to reject the
sibling call?  If not, then maybe the overlap routine should return a
tri-state.  No overlap, overlap, don't know.  The last would be used
when the two addresses use a different register.

Ok, here is my attempt at that. The overlap functions returns -2 when it
cannot staticall compare the
two pointers (i.e. when the base registers are different) and the caller
then disables sibcalls.
The code in calls.c that calls this code will undo any emitted
instructions in the meantime if sibcall
optimisation fails. This required me to change the type of
emit_push_insn to bool and add an extra
parameter, so this patch touches a bit more code than the original version.

Bootstrapped on x86_64 and tested on arm. The testcase in this PR still
performs a sibcall correctly on arm.

What do you think of this?

Thanks,
Kyrill


2015-05-11  Kyrylo Tkachov <kyrylo.tkachov@arm.com>

     PR target/65358
     * expr.c (memory_load_overlap): New function.
     (emit_push_insn): When pushing partial args to the stack would
     clobber the register part load the overlapping part into a pseudo
     and put it into the hard reg after pushing.  Change return type
     to bool.  Add bool argument.
     * expr.h (emit_push_insn): Change return type to bool.
     Add bool argument.
     * calls.c (expand_call): Cancel sibcall optimisation when encountering
     partial argument on targets with ARGS_GROW_DOWNWARD and
     !STACK_GROWS_DOWNWARD.
     (emit_library_call_value_1): Update callsite of emit_push_insn.
     (store_one_arg): Likewise.


2015-05-11  Honggyu Kim <hong.gyu.kim@lge.com>

     PR target/65358
     * gcc.dg/pr65358.c: New test.


Jeff




expr.patch


commit 5b596f10846b6d3b143442a306801c8262d8b10a
Author: Kyrylo Tkachov<kyrylo.tkachov@arm.com>
Date:   Wed Mar 18 13:42:37 2015 +0000

     [expr.c] PR 65358 Avoid clobbering partial argument during sibcall

diff --git a/gcc/calls.c b/gcc/calls.c
index caa7d60..81ef2c9 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -3225,6 +3225,13 @@ expand_call (tree exp, rtx target, int ignore)
  	    {
  	      rtx_insn *before_arg = get_last_insn ();

+	     /* On targets with weird calling conventions (e.g. PA) it's
+		hard to ensure that all cases of argument overlap between
+		stack and registers work.  Play it safe and bail out.  */
+#if defined (ARGS_GROW_DOWNWARD) && !defined (STACK_GROWS_DOWNWARD)
+	      sibcall_failure = 1;
+	      break;
+#endif
So we're trying to get away from this kind of conditional compilation.

Instead we want to write

if (ARGS_GROW_DOWNWARD && !STACK_GROWS_DOWNWARD)

ARGS_GROW_DOWNWARD is already a testable value. But STACK_GROWS_DOWNWARD is not. The way folks have been dealing with this is something like this after the #includes:

/* Redefine STACK_GROWS_DOWNWARD in terms of 0 or 1.  */
#ifdef STACK_GROWS_DOWNWARD
# undef STACK_GROWS_DOWNWARD
# define STACK_GROWS_DOWNWARD 1
#else
# define STACK_GROWS_DOWNWARD 0
#endif


With that in place you can change the test into the more desirable
if (ARGS_GROW_DOWNWARD && !STACK_GROWS_DOWNWARD)



diff --git a/gcc/expr.c b/gcc/expr.c
index 25aa11f..712fa0b 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -4121,12 +4121,35 @@ emit_single_push_insn (machine_mode mode, rtx x, tree type)
  }
  #endif

+/* If reading SIZE bytes from X will end up reading from
+   Y return the number of bytes that overlap.  Return -1
+   if there is no overlap or -2 if we can't determing
s/determing/determine/

+   partial argument during a sibcall optimisation (as specified by
s/optimisation/optimization/


I guess I'm still not real comfortable that we've got the issues here resolved and if we're going to try and support this case, then re-using the existing infrastructure would be better.

I'll approve with the changes noted above, but would ask that you look into trying to re-use the existing infrastructure as a follow-up.

Jeff



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]