From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 64725 invoked by alias); 5 May 2015 12:46:59 -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 64709 invoked by uid 89); 5 May 2015 12:46:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 05 May 2015 12:46:56 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D65BE29; Tue, 5 May 2015 05:46:24 -0700 (PDT) Received: from e105689-lin.cambridge.arm.com (e105689-lin.cambridge.arm.com [10.2.207.32]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4F3CE3F212; Tue, 5 May 2015 05:46:54 -0700 (PDT) Message-ID: <5548BBBC.7060000@foss.arm.com> Date: Tue, 05 May 2015 12:46:00 -0000 From: Richard Earnshaw User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Richard Biener , Jakub Jelinek CC: gcc-patches@gcc.gnu.org, Martin Jambor , Alan Lawrence Subject: Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956) 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> In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00319.txt.bz2 On 05/05/15 13:37, Richard Biener wrote: > 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. > > As is int foo2 (int a, int b); void bar2 (alignedint x) { foo2 (1, x); } ... R