From patchwork Mon Jun 14 08:39:44 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Bingfeng Mei X-Patchwork-Id: 55492 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 2A65B1007D1 for ; Mon, 14 Jun 2010 18:40:40 +1000 (EST) Received: (qmail 28432 invoked by alias); 14 Jun 2010 08:40:32 -0000 Received: (qmail 28414 invoked by uid 22791); 14 Jun 2010 08:40:29 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL, BAYES_00, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mms1.broadcom.com (HELO mms1.broadcom.com) (216.31.210.17) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 14 Jun 2010 08:40:23 +0000 Received: from [10.16.192.224] by mms1.broadcom.com with ESMTP (Broadcom SMTP Relay (Email Firewall v6.3.2)); Mon, 14 Jun 2010 01:40:10 -0700 X-Server-Uuid: 02CED230-5797-4B57-9875-D5D2FEE4708A Received: from SJEXCHCCR02.corp.ad.broadcom.com ([10.16.192.130]) by SJEXCHHUB01.corp.ad.broadcom.com ([10.16.192.224]) with mapi; Mon, 14 Jun 2010 01:40:10 -0700 From: "Bingfeng Mei" To: "Richard Guenther" cc: "gcc-patches@gcc.gnu.org" , "Jan Hubicka" Date: Mon, 14 Jun 2010 01:39:44 -0700 Subject: RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. Message-ID: <7FB04A5C213E9943A72EE127DB74F0ADA6667D06B5@SJEXCHCCR02.corp.ad.broadcom.com> References: <7FB04A5C213E9943A72EE127DB74F0ADA66675B3BB@SJEXCHCCR02.corp.ad.broadcom.com> <7FB04A5C213E9943A72EE127DB74F0ADA66675B3DF@SJEXCHCCR02.corp.ad.broadcom.com> In-Reply-To: x-cr-puzzleid: {723F8455-81EC-4928-AAF6-96F4A35C09AF} x-cr-hashedpuzzle: B1Nd DEFZ Gd1c Hlu2 HwP2 ITkL Mofh NucX XUfs Z1Av be5D c2fz fyHg gUfk h0ut iXQP; 3; ZwBjAGMALQBwAGEAdABjAGgAZQBzAEAAZwBjAGMALgBnAG4AdQAuAG8AcgBnADsAaAB1AGIAaQBjAGsAYQBAAHUAYwB3AC4AYwB6ADsAcgBpAGMAaABhAHIAZAAuAGcAdQBlAG4AdABoAGUAcgBAAGcAbQBhAGkAbAAuAGMAbwBtAA==; Sosha1_v1; 7; {723F8455-81EC-4928-AAF6-96F4A35C09AF}; YgBtAGUAaQBAAGIAcgBvAGEAZABjAG8AbQAuAGMAbwBtAA==; Mon, 14 Jun 2010 08:39:46 GMT; UgBFADoAIABbAFAAQQBUAEMASAAsACAATABUAE8AXQAgAGEAZABkACAAZQB4AHQAZQByAG4AYQBsAGwAeQBfAHYAaQBzAGkAYgBsAGUAIABhAHQAdAByAGkAYgB1AHQAZQAgAHcAaABlAG4AIABuAGUAYwBlAHMAcwBhAHIAeQAgAHcAaQB0AGgAIAAtAGYAdwBoAG8AbABlAC0AcAByAG8AZwByAGEAbQAgAGEAbgBkACAAcgBlAHMAbwBsAHUAdABpAG8AbgAgAGYAaQBsAGUALgA= MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Hi, Richard, Here is the updated patch. The flags are set instead of attributes now. The check is moved to the end of lto_symtab_merge_decls_1. For the DECL_COMM, since internal resolver is always used due to your workaround for gold plugin, These variables would still need explicit externally_visible attributes. Bootstrapped and tested. Cheers, Bingfeng. 2010-06-11 Bingfeng Mei * lto-symbtab.c (lto_symtab_merge_decls_1): Set externally_visible flags for symbols of LDPR_PREVAILING_DEF when compiling with -fwhole-program (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY for internal resolver * ipa.c (function_and_variable_visibility): Set externally_visible flags only if they are false. This allows flags to be passed from > -----Original Message----- > From: Richard Guenther [mailto:richard.guenther@gmail.com] > Sent: 09 June 2010 16:26 > To: Bingfeng Mei > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH, LTO] add externally_visible attribute when > necessary with -fwhole-program and resolution file. > > On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei wrote: > > I added an attribute because -fwhole-program/externally_visible is > handled in ipa.c > > > > ... > >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES (node- > >decl))) > >    return true; > > ... > > > > Adding attribute seems cleaner than changing flags, otherwise I need > to change > > handling in ipa.c as well. > > True, but there is an externally_visible flag in cgraph_node, > so I guess that attribute lookup is bogus. > > Richard. > > > Bingfeng > > > >> -----Original Message----- > >> From: Richard Guenther [mailto:richard.guenther@gmail.com] > >> Sent: 09 June 2010 16:02 > >> To: Bingfeng Mei > >> Cc: gcc-patches@gcc.gnu.org > >> Subject: Re: [PATCH, LTO] add externally_visible attribute when > >> necessary with -fwhole-program and resolution file. > >> > >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei > wrote: > >> > Hi, > >> > This patch addresses issue discussed in > >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html > >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html > >> > > >> > With the patch, any declaration which is resolved as > >> LDPR_PREVAILING_DEF > >> > and compiled with -fwhole-program is annotated with > >> > attribute "externally_visible" if it doesn't exist already. > >> > This eliminates the error-prone process of manual annotation > >> > of the attribute when compiling mixed LTO/non-LTO applications. > >> > > >> > For the following test files: > >> > a.c > >> > > >> > #include > >> > #include > >> > extern int foo(int); > >> > /* Called by b.c, should not be optimized by -fwhole-program */ > >> > void bar() > >> > { > >> >  printf("bar\n"); > >> > } > >> > /* Not used by others, should be optimized out by -fwhole- > program*/ > >> > void bar2() > >> > { > >> >  printf("bar2\n"); > >> > } > >> > extern int src[], dst[]; > >> > int vvvvvv; > >> > int main() > >> > { > >> >  int ret; > >> >  vvvvvv = 12; > >> >  ret = foo(20); > >> >  bar2(); > >> >  memcpy(dst, src, 100); > >> >  return ret + 3; > >> > } > >> > > >> > b.c > >> > > >> > #include > >> > int src[100]; > >> > int dst[100]; > >> > extern int vvvvvv; > >> > extern void bar(); > >> > int foo(int c) > >> > { > >> >  printf("Hello world: %d\n", c); > >> >  bar(); > >> >  return 1000 + vvvvvv; > >> > } > >> > > >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto > >> > ~/work/install-x86/bin/gcc  b.c -O2 -c > >> > ar cru libb.a b.o > >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-linker- > plugin > >> -o f -fwhole-program > >> > > >> > The code is compiled and linked correctly. bar & vvvvvv don't > become > >> static function > >> > and cause link errors, whereas bar2 is inlined as expected. > >> > > >> > > >> > Bootstrapped and tested on x86_64-unknown-linux-gnu. > >> > > >> > Cheers, > >> > Bingfeng Mei > >> > > >> > 2010-06-09  Bingfeng Mei > >> > > >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add > >> externally_visible > >> >        attribute for declaration of LDPR_PREVAILING_DEF when > >> compiling with > >> >        -fwhole-program > >> > > >> > > >> > Index: lto-symtab.c > >> > > =================================================================== > >> > --- lto-symtab.c        (revision 160463) > >> > +++ lto-symtab.c        (working copy) > >> > @@ -476,7 +476,19 @@ > >> > > >> >   /* If the chain is already resolved there is nothing else to > >> do.  */ > >> >   if (e->resolution != LDPR_UNKNOWN) > >> > -    return; > >> > +    { > >> > +      /* Add externally_visible attribute for declaration of > >> LDPR_PREVAILING_DEF */ > >> > +      if (e->resolution == LDPR_PREVAILING_DEF && > flag_whole_program) > >> > +        { > >> > +          if (!lookup_attribute ("externally_visible", > >> DECL_ATTRIBUTES (e->decl))) > >> > +            { > >> > +              DECL_ATTRIBUTES (e->decl) > >> > +                = tree_cons (get_identifier > ("externally_visible"), > >> NULL_TREE, > >> > +                             DECL_ATTRIBUTES (e->decl)); > >> > >> I don't think we need to add an attribute here but we can play > >> with some cgraph flags which is cheaper. > >> > >> Also I think this isn't really correct - not everything that > prevails > >> needs to be externally visible (in fact, you seem to simply > >> remove the effect of -fwhole-program completely). > >> > >> A testcase that should still work is > >> > >> t1.c: > >> void foo(void) { bar (); } > >> t2.c > >> extern void foo (void); > >> void bar (void) {} > >> void eliminate_me (void) {} > >> int main() > >> { > >>    foo(); > >> } > >> > >> and eliminate_me should still be eliminated with -fwhole-program > >> if you do > >> > >> gcc -c t1.c > >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin > >> > >> Thus, the resolution file probably does not have the information > >> you need (a list of references from outside of the LTO file set). > >> > >> Richard. > > > > > > Index: lto-symtab.c =================================================================== --- lto-symtab.c (revision 160529) +++ lto-symtab.c (working copy) @@ -530,11 +530,21 @@ return; found: - if (TREE_CODE (prevailing->decl) == VAR_DECL - && TREE_READONLY (prevailing->decl)) + /* If current lto files represent the whole program, + it is correct to use LDPR_PREVALING_DEF_IRONLY. + If current lto files are part of whole program, internal + resolver doesn't know if it is LDPR_PREVAILING_DEF + or LDPR_PREVAILING_DEF_IRONLY. Use IRONLY conforms to + using -fwhole-program. Otherwise, it doesn't + matter using either LDPR_PREVAILING_DEF or + LDPR_PREVAILING_DEF_IRONLY + + FIXME: above workaround due to gold plugin makes some + variables IRONLY, which are indeed PREVAILING_DEF in + resolution file. These variables still need manual + externally_visible attribute + */ prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; - else - prevailing->resolution = LDPR_PREVAILING_DEF; } /* Merge all decls in the symbol table chain to the prevailing decl and @@ -698,6 +708,25 @@ && TREE_CODE (prevailing->decl) != VAR_DECL) prevailing->next = NULL; + + /* Add externally_visible attribute for declaration of LDPR_PREVAILING_DEF */ + if (flag_whole_program) + { + if (prevailing->resolution == LDPR_PREVAILING_DEF) + { + if (TREE_CODE (prevailing->decl) == FUNCTION_DECL) + prevailing->node->local.externally_visible = true; + else + prevailing->vnode->externally_visible = true; + } + else if (prevailing->resolution == LDPR_PREVAILING_DEF_IRONLY) + { + if (TREE_CODE (prevailing->decl) == FUNCTION_DECL) + prevailing->node->local.externally_visible = false; + else + prevailing->vnode->externally_visible = false; + } + } return 1; } Index: ipa.c =================================================================== --- ipa.c (revision 160529) +++ ipa.c (working copy) @@ -665,13 +665,12 @@ } gcc_assert ((!DECL_WEAK (node->decl) && !DECL_COMDAT (node->decl)) || TREE_PUBLIC (node->decl) || DECL_EXTERNAL (node->decl)); - if (cgraph_externally_visible_p (node, whole_program)) + if (!node->local.externally_visible + && cgraph_externally_visible_p (node, whole_program)) { gcc_assert (!node->global.inlined_to); node->local.externally_visible = true; } - else - node->local.externally_visible = false; if (!node->local.externally_visible && node->analyzed && !DECL_EXTERNAL (node->decl)) { @@ -721,7 +720,8 @@ { if (!vnode->finalized) continue; - if (vnode->needed + if (!vnode->externally_visible + && vnode->needed && (DECL_COMDAT (vnode->decl) || TREE_PUBLIC (vnode->decl)) && (!whole_program /* We can privatize comdat readonly variables whose address is not taken, @@ -732,8 +732,6 @@ || lookup_attribute ("externally_visible", DECL_ATTRIBUTES (vnode->decl)))) vnode->externally_visible = true; - else - vnode->externally_visible = false; if (!vnode->externally_visible) { gcc_assert (whole_program || !TREE_PUBLIC (vnode->decl));