From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 74782 invoked by alias); 4 May 2015 08:11: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 73881 invoked by uid 89); 4 May 2015 08:11:18 -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_50,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; Mon, 04 May 2015 08:11:17 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 0B2C3ACE9; Mon, 4 May 2015 08:11:14 +0000 (UTC) Date: Mon, 04 May 2015 08:11:00 -0000 From: Richard Biener To: Jakub Jelinek cc: gcc-patches@gcc.gnu.org, Martin Jambor Subject: Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956) In-Reply-To: <20150502082437.GW1751@tucnak.redhat.com> Message-ID: References: <20150502082437.GW1751@tucnak.redhat.com> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2015-05/txt/msg00189.txt.bz2 On Sat, 2 May 2015, Jakub Jelinek wrote: > Hi! > > This is an attempt to fix the following testcase (reduced from gsoap) > similarly how you've fixed another issue with r221795 other AAPCS > regressions introduced with r221348 change. > This patch passed bootstrap/regtest on > {x86_64,i686,armv7hl,aarch64,powerpc64{,le},s390{,x}}-linux. > > Though, it still doesn't fix profiledbootstrap on armv7hl that is broken > since r221348, so other issues are lurking in there, and I must say > I'm not entirely sure about this, because it changes alignment even when > the original access had higher alignment. > > I was trying something like: > struct B { char *a, *b; }; > typedef struct B C __attribute__((aligned (8))); > struct A { C a; int b; long long c; }; > char v[3]; > > __attribute__((noinline, noclone)) void > fn1 (C x, C y) > { > if (x.a != &v[1] || y.a != &v[2]) > __builtin_abort (); > v[1]++; > } > > __attribute__((noinline, noclone)) int > fn2 (C x) > { > asm volatile ("" : "+g" (x.a) : : "memory"); > asm volatile ("" : "+g" (x.b) : : "memory"); > return x.a == &v[0]; > } > > __attribute__((noinline, noclone)) void > fn3 (const char *x) > { > if (x[0] != 0) > __builtin_abort (); > } > > static struct A > foo (const char *x, struct A y, struct A z) > { > struct A r = { { 0, 0 }, 0, 0 }; > if (y.b && z.b) > { > if (fn2 (y.a) && fn2 (z.a)) > switch (x[0]) > { > case '|': > break; > default: > fn3 (x); > } > fn1 (y.a, z.a); > } > return r; > } > > __attribute__((noinline, noclone)) int > bar (int x, struct A *y) > { > switch (x) > { > case 219: > foo ("+", y[-2], y[0]); > case 220: > foo ("-", y[-2], y[0]); > } > } > > int > main () > { > struct A a[3] = { { { &v[1], &v[0] }, 1, 1LL }, > { { &v[0], &v[0] }, 0, 0LL }, > { { &v[2], &v[0] }, 2, 2LL } }; > bar (220, a + 2); > if (v[1] != 1) > __builtin_abort (); > return 0; > } > > and this patch indeed changes the register passing, eventhough it probably > shouldn't (though, the testcase doesn't fail). Wouldn't it be possible to > preserve the original type (before we call build_aligned_type on it) > somewhere in SRA data structures, perhaps keep expr (the new MEM_REF) use > the aligned type, but type field be the non-aligned one? Not sure how this helps when SRA tears apart the parameter. That is, isn't the important thing that both the IPA modified function argument types/decls have the same type as the types of the parameters SRA ends up passing? (as far as alignment goes?) Yes, of course using "natural" alignment makes sure that the backend can handle alignment properly and we don't run into oddball bugs here. > 2015-05-02 Jakub Jelinek > > PR target/65956 > * tree-sra.c (turn_representatives_into_adjustments): For > adj.type, use TYPE_MAIN_VARIANT of repr->type with TYPE_QUALS. > > * gcc.c-torture/execute/pr65956.c: New test. > > --- gcc/tree-sra.c.jj 2015-04-20 14:35:47.000000000 +0200 > +++ gcc/tree-sra.c 2015-05-01 01:08:34.092636496 +0200 > @@ -4427,7 +4427,11 @@ turn_representatives_into_adjustments (v > gcc_assert (repr->base == parm); > adj.base_index = index; > adj.base = repr->base; > - adj.type = repr->type; > + /* Drop any special alignment on the type if it's not on the > + main variant. This avoids issues with weirdo ABIs like > + AAPCS. */ > + adj.type = build_qualified_type (TYPE_MAIN_VARIANT (repr->type), > + TYPE_QUALS (repr->type)); So - this changes the function argument type of the clone? Does it also change the type of the value we pass to the function? That is, why drop the alignment here but not avoid attaching it to repr->type in the first place as my fix for the other issue did? Doesn't the above just make it inconsistent by default? There is also the correctness issue of under-aligned types (which was what the original code using build_aligned_type cared for - before I "fixed" it to also preserve over-alignment). That said - somewhere we create the register we use for passing the argument, and only the type of that register needs fixing IMHO. We also have ptype = adj->type; if (is_gimple_reg_type (ptype)) { unsigned malign = GET_MODE_ALIGNMENT (TYPE_MODE (ptype)); if (TYPE_ALIGN (ptype) < malign) ptype = build_aligned_type (ptype, malign); in ipa_modify_formal_parameters. That looks odd for by-value passing as well. When modifying the function bodies we simply take what was set in ->new_decl which we'd populate above in ipa_modify_formal_parameters. It seems to me that ipa_modify_expr should look to preserve alignment at the callers site (for loading into the regs we pass) for non-reference passing. Esp. if (cand->by_ref) src = build_simple_mem_ref (cand->new_decl); looks bogus in this regard (unless we make sure that the pointed-to type of cand->new_decl has all under-/over-alignment applied properly). It would probably be clearer to record "alignment of original memory access" in the data structures and apply that only to memory references we generate, not to registers we eventually end up creating. To me all of that IPA SRA stuff is a bit of a twisted maze ... but we definitely want to root out any use of over-/under-alignment we generate for PARM_DECLs and decls/SSA names we end up passing by value. I'm not 100% sure your patch walks in that direction without changing alignment of memory references we eventually generate. But maybe Martin can shed some light on this... Thanks, Richard. > adj.alias_ptr_type = reference_alias_ptr_type (repr->expr); > adj.offset = repr->offset; > adj.by_ref = (POINTER_TYPE_P (TREE_TYPE (repr->base)) > --- gcc/testsuite/gcc.c-torture/execute/pr65956.c.jj 2015-05-01 10:32:34.730150257 +0200 > +++ gcc/testsuite/gcc.c-torture/execute/pr65956.c 2015-05-01 10:32:13.000000000 +0200 > @@ -0,0 +1,67 @@ > +/* PR target/65956 */ > + > +struct A { char *a; int b; long long c; }; > +char v[3]; > + > +__attribute__((noinline, noclone)) void > +fn1 (char *x, char *y) > +{ > + if (x != &v[1] || y != &v[2]) > + __builtin_abort (); > + v[1]++; > +} > + > +__attribute__((noinline, noclone)) int > +fn2 (char *x) > +{ > + asm volatile ("" : "+g" (x) : : "memory"); > + return x == &v[0]; > +} > + > +__attribute__((noinline, noclone)) void > +fn3 (const char *x) > +{ > + if (x[0] != 0) > + __builtin_abort (); > +} > + > +static struct A > +foo (const char *x, struct A y, struct A z) > +{ > + struct A r = { 0, 0, 0 }; > + if (y.b && z.b) > + { > + if (fn2 (y.a) && fn2 (z.a)) > + switch (x[0]) > + { > + case '|': > + break; > + default: > + fn3 (x); > + } > + fn1 (y.a, z.a); > + } > + return r; > +} > + > +__attribute__((noinline, noclone)) int > +bar (int x, struct A *y) > +{ > + switch (x) > + { > + case 219: > + foo ("+", y[-2], y[0]); > + case 220: > + foo ("-", y[-2], y[0]); > + } > +} > + > +int > +main () > +{ > + struct A a[3] = { { &v[1], 1, 1LL }, { &v[0], 0, 0LL }, { &v[2], 2, 2LL } }; > + bar (220, a + 2); > + if (v[1] != 1) > + __builtin_abort (); > + return 0; > +} > > Jakub > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)