From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126151 invoked by alias); 17 Apr 2015 17:26:19 -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 126140 invoked by uid 89); 17 Apr 2015 17:26:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no 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; Fri, 17 Apr 2015 17:26:17 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 51CFB8E3EA; Fri, 17 Apr 2015 17:26:16 +0000 (UTC) Received: from localhost.localdomain (ovpn-113-81.phx2.redhat.com [10.3.113.81]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3HHQElg001597; Fri, 17 Apr 2015 13:26:15 -0400 Message-ID: <55314236.2000806@redhat.com> Date: Fri, 17 Apr 2015 17:26: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> In-Reply-To: <550ADF8F.7030300@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg00913.txt.bz2 On 03/19/2015 08:39 AM, Kyrill Tkachov wrote: > Hi all, > > This patch fixes PR 65358. For details look at the excellent write-up > by Honggyu in bugzilla. The problem is that we're trying to pass a struct > partially on the stack and partially in regs during a tail-call > optimisation > but the struct we're passing is also a partial incoming arg though the > split > between stack and regs is different from its outgoing usage. > > The emit_push_insn code ends up doing a block move for the on-stack part > but > ends up overwriting the part that needs to be loaded into regs. > My first thought was to just load the regs part first and then do the stack > part but that doesn't work as multiple comments in that function indicate > (the block move being expanded to movmem or other functions being one of > the > reasons). > > My proposed solution is to detect when the overlap happens, find the > overlapping region and load it before the stack pushing into pseudos and > after the stack pushing is done move the overlapping values from the > pseudos > into the hard argument regs that they're supposed to go. > > That way this new functionality should only ever be triggered when there's > the overlap in this PR (causing wrong-code) and shouldn't affect codegen > anywhere else. > > Bootstrapped and tested on arm-none-linux-gnueabihf, aarch64-none-linux-gnu > and x86_64-linux-gnu. > > According to the PR this appears at least as far back 4.6 so this isn't a > regression on the release branches, but it is a wrong-code bug. > > I'll let Honggyu upstream the testcase separately > (https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00984.html) > > I'll be testing this on the 4.8 and 4.9 branches. > Thoughts on this approach? > > Thanks, > Kyrill > > 2015-03-19 Kyrylo Tkachov > > PR middle-end/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. > > expr.patch > > > commit 490c5f2074d76a2927afaea99e4dd0bacccb413c > 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/expr.c b/gcc/expr.c > index dc13a14..d3b9156 100644 > --- a/gcc/expr.c > +++ b/gcc/expr.c > @@ -4121,6 +4121,25 @@ emit_single_push_insn (machine_mode mode, rtx x, tree type) > } > #endif > > +/* Add SIZE to X and check whether it's greater than Y. > + If it is, return the constant amount by which it's greater or smaller. > + If the two are not statically comparable (for example, X and Y contain > + different registers) return -1. This is used in expand_push_insn to > + figure out if reading SIZE bytes from location X will end up reading from > + location Y. */ > + > +static int > +memory_load_overlap (rtx x, rtx y, HOST_WIDE_INT size) > +{ > + rtx tmp = plus_constant (Pmode, x, size); > + rtx sub = simplify_gen_binary (MINUS, Pmode, tmp, y); > + > + if (!CONST_INT_P (sub)) > + return -1; > + > + return INTVAL (sub); > +} Hmmm, so what happens if the difference is < 0? I'd be a bit worried about that case for the PA (for example). So how about asserting that the INTVAL is >= 0 prior to returning so that we catch that case if it ever occurs? OK for the trunk with the added assert. Please commit the testcase from Honggyu at the same time you commit the patch. Let's let it simmer for a while on the trunk before considering it to be backported. jeff