From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75463 invoked by alias); 12 May 2015 22:04:52 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 75449 invoked by uid 89); 12 May 2015 22:04:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.5 required=5.0 tests=AWL,BAYES_50,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 12 May 2015 22:04:50 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4CM4ldF014582 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 12 May 2015 18:04:47 -0400 Received: from localhost.localdomain (ovpn-113-21.phx2.redhat.com [10.3.113.21]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4CM4lYQ030226; Tue, 12 May 2015 18:04:47 -0400 Message-ID: <555278FE.90603@redhat.com> Date: Tue, 12 May 2015 22:12:00 -0000 From: Jeff Law User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Kyrill Tkachov , GCC Patches CC: Honggyu Kim Subject: Re: [PATCH][expr.c] PR 65358 Avoid clobbering partial argument during sibcall References: <550ADF8F.7030300@arm.com> <55314236.2000806@redhat.com> <5534B811.2020107@arm.com> <55353F25.9010906@redhat.com> <55360AA0.9070106@arm.com> <55365A06.7010408@redhat.com> <553689F0.5090606@arm.com> <553E9869.30502@redhat.com> <553F58BB.4070508@foss.arm.com> <5543CB30.5060403@redhat.com> <5550764C.1070502@foss.arm.com> In-Reply-To: <5550764C.1070502@foss.arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg01167.txt.bz2 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 > > 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 > > PR target/65358 > * gcc.dg/pr65358.c: New test. > >> >> Jeff >> >> > > > expr.patch > > > commit 5b596f10846b6d3b143442a306801c8262d8b10a > Author: Kyrylo Tkachov > 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