From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 39469 invoked by alias); 5 May 2015 12:37:30 -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 39458 invoked by uid 89); 5 May 2015 12:37:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx2.suse.de Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Tue, 05 May 2015 12:37:19 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 31010ACCD; Tue, 5 May 2015 12:37:16 +0000 (UTC) User-Agent: K-9 Mail for Android In-Reply-To: <5548A327.3080103@foss.arm.com> References: <20150502082437.GW1751@tucnak.redhat.com> <20150504150011.GD1751@tucnak.redhat.com> <20150505073228.GH1751@tucnak.redhat.com> <5548A174.4010208@foss.arm.com> <5548A327.3080103@foss.arm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=UTF-8 Subject: Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956) From: Richard Biener Date: Tue, 05 May 2015 12:37:00 -0000 To: Richard Earnshaw ,Jakub Jelinek CC: gcc-patches@gcc.gnu.org,Martin Jambor ,Alan Lawrence Message-ID: X-SW-Source: 2015-05/txt/msg00315.txt.bz2 On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw wrote: >On 05/05/15 11:54, Richard Earnshaw wrote: >> On 05/05/15 08:32, Jakub Jelinek wrote: >>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote: >>>> So at least changing arm_needs_doubleword_align for non-aggregates >would >>>> likely not break anything that hasn't been broken already and would >unbreak >>>> the majority of cases. >>> >>> Attached (untested so far). It indeed changes code generated for >>> over-aligned va_arg, but as I believe you can't properly pass those >in the >>> ... caller, this should just fix it so that va_arg handling matches >the >>> caller (and likewise for callees for named argument passing). >>> >>>> The following testcase shows that eipa_sra changes alignment even >for the >>>> aggregates. Change aligned (8) to aligned (4) to see another >possibility. >>> >>> Actually I misread it, for the aggregates esra actually doesn't >change >>> anything, which is the reason why the testcase doesn't fail. >>> The problem with the scalars is that esra first changed it to the >>> over-aligned MEM_REFs and then later on eipa_sra used the types of >the >>> MEM_REFs created by esra. >>> >>> 2015-05-05 Jakub Jelinek >>> >>> PR target/65956 >>> * config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate >>> types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type >itself. >>> >>> * gcc.c-torture/execute/pr65956.c: New test. >>> >>> --- gcc/config/arm/arm.c.jj 2015-05-04 21:51:42.000000000 +0200 >>> +++ gcc/config/arm/arm.c 2015-05-05 09:20:52.481693337 +0200 >>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG >>> static bool >>> arm_needs_doubleword_align (machine_mode mode, const_tree type) >>> { >>> - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY >>> - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY)); >>> + if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY) >>> + return true; >> >> I don't think this is right (though I suspect the existing code has >the >> same problem). We should only look at mode if there is no type >> information. The problem is that GCC has a nasty habit of assigning >> real machine modes to things that are really BLKmode and we've run >into >> several cases where this has royally screwed things up. So for >> consistency in the ARM back-end we are careful to only use mode when >> type is NULL (=> it's a libcall). >> >>> + if (type == NULL_TREE) >>> + return false; >>> + if (AGGREGATE_TYPE_P (type)) >>> + return TYPE_ALIGN (type) > PARM_BOUNDARY; >>> + return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY; >>> } >>> >> >> >> >> It ought to be possible to re-order this, though, to >> >> static bool >> arm_needs_doubleword_align (machine_mode mode, const_tree type) >> { >> - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY >> - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY)); >> + if (type != NULL_TREE) >> + { >> + if (AGGREGATE_TYPE_P (type)) >> + return TYPE_ALIGN (type) > PARM_BOUNDARY; >> + return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY; >> + } >> + return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY); >> } >> >> >> Either way, this would need careful cross-testing against an existing >> compiler. >> > >It looks as though either patch would cause ABI incompatibility for > >typedef int alignedint __attribute__((aligned((8)))); > >int __attribute__((weak)) foo (int a, alignedint b) >{return b;} > >void bar (alignedint x) >{ > foo (1, x); >} > >Where currently gcc uses r2 as the argument register for b in foo. And for foo (1,2) or an int typed 2nd arg? Richard. >R.