| Message ID | 7FB04A5C213E9943A72EE127DB74F0ADA6667D06B5@SJEXCHCCR02.corp.ad.broadcom.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 14, 2010 at 10:39 AM, Bingfeng Mei <bmei@broadcom.com> wrote: > 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. Can you amend the docs that talk about -flto + -fwhole-program accordingly? I'd like Honza to go over the cgraph related changes, otherwise the patch looks good with ... > Bootstrapped and tested. > > Cheers, > Bingfeng. > > 2010-06-11 Bingfeng Mei <bmei@broadcom.com> > > * 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 > > > 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 > + */ Full-stop at the end of the sentence, */ not on the next line. Also two spaces after each '.' > 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; > > + No extra vertical space please. > + /* Add externally_visible attribute for declaration of LDPR_PREVAILING_DEF */ Adjust the comment, to sth like "In whole-program mode mark LDPR_PREVAILING_DEFs as externally visible. " > + 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; > + } > + } Honza will tell you if the above is correct, I am not 100% sure. Did you verify we generate the same code with and without your patch when all symbols are resolved inside the IL? Thanks, Richard. > 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)); > >> -----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 <bmei@broadcom.com> 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 <bmei@broadcom.com> >> 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 <string.h> >> >> > #include <stdio.h> >> >> > 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 <stdio.h> >> >> > 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 <bmei@broadcom.com> >> >> > >> >> > * 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. >> > >> > >> > > > >
I will amend the LTO documents. With/without my patch, GCC produces the same results compiling for IR only for my examples. And it passes LTO tests. However, bar2 and foo functions are not inlined as I expected using GNU ld. I believe this is due to the "externally_visible" flags are improperly passed from compile-time to link time as discussed in previous mails in this thread, and this won't be affected by my patch. I will send a separate patch. Cheers, Bingfeng > -----Original Message----- > From: Richard Guenther [mailto:richard.guenther@gmail.com] > Sent: 14 June 2010 10:25 > To: Bingfeng Mei > Cc: gcc-patches@gcc.gnu.org; Jan Hubicka > Subject: Re: [PATCH, LTO] add externally_visible attribute when > necessary with -fwhole-program and resolution file. > > On Mon, Jun 14, 2010 at 10:39 AM, Bingfeng Mei <bmei@broadcom.com> > wrote: > > 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. > > Can you amend the docs that talk about -flto + -fwhole-program > accordingly? > > I'd like Honza to go over the cgraph related changes, otherwise > the patch looks good with ... > > > Bootstrapped and tested. > > > > Cheers, > > Bingfeng. > > > > 2010-06-11 Bingfeng Mei <bmei@broadcom.com> > > > > * 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 > > > > > > 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 > > + */ > > Full-stop at the end of the sentence, */ not on the next line. Also > two spaces after each '.' > > > 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; > > > > + > > No extra vertical space please. > > > + /* Add externally_visible attribute for declaration of > LDPR_PREVAILING_DEF */ > > Adjust the comment, to sth like "In whole-program mode mark > LDPR_PREVAILING_DEFs as externally visible. " > > > + 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; > > + } > > + } > > Honza will tell you if the above is correct, I am not 100% sure. > > Did you verify we generate the same code with and without your > patch when all symbols are resolved inside the IL? > > Thanks, > Richard. > > > 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)); > > > >> -----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 <bmei@broadcom.com> > 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 <bmei@broadcom.com> > >> 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 <string.h> > >> >> > #include <stdio.h> > >> >> > 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 <stdio.h> > >> >> > 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 <bmei@broadcom.com> > >> >> > > >> >> > * 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. > >> > > >> > > >> > > > > > > > #include <stdio.h> 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; } #include <string.h> #include <stdio.h> 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; }
Honza, Could you have a look at cgraph related changes as suggested by Richard? Thanks! Bingfeng > -----Original Message----- > From: Richard Guenther [mailto:richard.guenther@gmail.com] > Sent: 14 June 2010 10:25 > To: Bingfeng Mei > Cc: gcc-patches@gcc.gnu.org; Jan Hubicka > Subject: Re: [PATCH, LTO] add externally_visible attribute when > necessary with -fwhole-program and resolution file. > > On Mon, Jun 14, 2010 at 10:39 AM, Bingfeng Mei <bmei@broadcom.com> > wrote: > > 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. > > Can you amend the docs that talk about -flto + -fwhole-program > accordingly? > > I'd like Honza to go over the cgraph related changes, otherwise > the patch looks good with ... > > > Bootstrapped and tested. > > > > Cheers, > > Bingfeng. > > > > 2010-06-11 Bingfeng Mei <bmei@broadcom.com> > > > > * 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 > > > > > > 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 > > + */ > > Full-stop at the end of the sentence, */ not on the next line. Also > two spaces after each '.' > > > 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; > > > > + > > No extra vertical space please. > > > + /* Add externally_visible attribute for declaration of > LDPR_PREVAILING_DEF */ > > Adjust the comment, to sth like "In whole-program mode mark > LDPR_PREVAILING_DEFs as externally visible. " > > > + 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; > > + } > > + } > > Honza will tell you if the above is correct, I am not 100% sure. > > Did you verify we generate the same code with and without your > patch when all symbols are resolved inside the IL? > > Thanks, > Richard. > > > 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)); > > > >> -----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 <bmei@broadcom.com> > 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 <bmei@broadcom.com> > >> 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 <string.h> > >> >> > #include <stdio.h> > >> >> > 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 <stdio.h> > >> >> > 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 <bmei@broadcom.com> > >> >> > > >> >> > * 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. > >> > > >> > > >> > > > > > > >
Honza, Could you have a look at the patch, especially on how cgraph related changes? Thanks, Bingfeng > -----Original Message----- > From: Richard Guenther [mailto:richard.guenther@gmail.com] > Sent: 14 June 2010 10:25 > To: Bingfeng Mei > Cc: gcc-patches@gcc.gnu.org; Jan Hubicka > Subject: Re: [PATCH, LTO] add externally_visible attribute when > necessary with -fwhole-program and resolution file. > > On Mon, Jun 14, 2010 at 10:39 AM, Bingfeng Mei <bmei@broadcom.com> > wrote: > > 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. > > Can you amend the docs that talk about -flto + -fwhole-program > accordingly? > > I'd like Honza to go over the cgraph related changes, otherwise > the patch looks good with ... > > > Bootstrapped and tested. > > > > Cheers, > > Bingfeng. > > > > 2010-06-11 Bingfeng Mei <bmei@broadcom.com> > > > > * 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 > > > > > > 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 > > + */ > > Full-stop at the end of the sentence, */ not on the next line. Also > two spaces after each '.' > > > 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; > > > > + > > No extra vertical space please. > > > + /* Add externally_visible attribute for declaration of > LDPR_PREVAILING_DEF */ > > Adjust the comment, to sth like "In whole-program mode mark > LDPR_PREVAILING_DEFs as externally visible. " > > > + 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; > > + } > > + } > > Honza will tell you if the above is correct, I am not 100% sure. > > Did you verify we generate the same code with and without your > patch when all symbols are resolved inside the IL? > > Thanks, > Richard. > > > 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)); > > > >> -----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 <bmei@broadcom.com> > 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 <bmei@broadcom.com> > >> 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 <string.h> > >> >> > #include <stdio.h> > >> >> > 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 <stdio.h> > >> >> > 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 <bmei@broadcom.com> > >> >> > > >> >> > * 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));
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 <bmei@broadcom.com> * 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 <bmei@broadcom.com> 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 <bmei@broadcom.com> > 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 <string.h> > >> > #include <stdio.h> > >> > 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 <stdio.h> > >> > 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 <bmei@broadcom.com> > >> > > >> > * 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. > > > > > >