* [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. @ 2010-06-09 15:02 Bingfeng Mei 2010-06-09 15:13 ` Richard Guenther 0 siblings, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-09 15:02 UTC (permalink / raw) To: gcc-patches; +Cc: Richard Guenther 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)); + } + } + return; + } /* Find the single non-replaceable prevailing symbol and diagnose ODR violations. */ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-09 15:02 [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file Bingfeng Mei @ 2010-06-09 15:13 ` Richard Guenther 2010-06-09 15:25 ` Bingfeng Mei 2010-06-09 15:32 ` Bingfeng Mei 0 siblings, 2 replies; 27+ messages in thread From: Richard Guenther @ 2010-06-09 15:13 UTC (permalink / raw) To: Bingfeng Mei; +Cc: gcc-patches 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-09 15:13 ` Richard Guenther @ 2010-06-09 15:25 ` Bingfeng Mei 2010-06-09 15:33 ` Richard Guenther 2010-06-09 15:32 ` Bingfeng Mei 1 sibling, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-09 15:25 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches > -----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. Then I also need to change relevant flag for variable declaration. Not sure what I should change. > > 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). According to doc, LDPR_PREVAILING_DEF is the definition accessed outside of LTO IR as opposed to LDPR_PREVAILING_DEF_IRONLY. So I think it should be externally visible. In my test file, bar2 is the similar as eliminate_me in your testcase. It is correctly eliminated with -fwhole-program. > > 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-09 15:25 ` Bingfeng Mei @ 2010-06-09 15:33 ` Richard Guenther 2010-06-09 15:35 ` Bingfeng Mei 0 siblings, 1 reply; 27+ messages in thread From: Richard Guenther @ 2010-06-09 15:33 UTC (permalink / raw) To: Bingfeng Mei; +Cc: gcc-patches, Jan Hubicka On Wed, Jun 9, 2010 at 5:14 PM, Bingfeng Mei <bmei@broadcom.com> wrote: > > >> -----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. > > Then I also need to change relevant flag for variable declaration. Not sure what > I should change. > >> >> 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). > > According to doc, LDPR_PREVAILING_DEF is the definition accessed outside > of LTO IR as opposed to LDPR_PREVAILING_DEF_IRONLY. So I think it should > be externally visible. In my test file, bar2 is the similar as eliminate_me > in your testcase. It is correctly eliminated with -fwhole-program. Oh, I see. (where's that doc btw?) I guess the internal resolver should then be updated to use IRONLY and applying external visibility should be done at the end of lto_symtab_merge_decls_1 then. Honza - what's the cgraph/varpool way of marking sth externally visible? Richard. >> >> 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. > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-09 15:33 ` Richard Guenther @ 2010-06-09 15:35 ` Bingfeng Mei 0 siblings, 0 replies; 27+ messages in thread From: Bingfeng Mei @ 2010-06-09 15:35 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches, Jan Hubicka > -----Original Message----- > From: Richard Guenther [mailto:richard.guenther@gmail.com] > Sent: 09 June 2010 16: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 Wed, Jun 9, 2010 at 5:14 PM, Bingfeng Mei <bmei@broadcom.com> wrote: > > > > > >> -----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. > > > > Then I also need to change relevant flag for variable declaration. > Not sure what > > I should change. > > > >> > >> 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). > > > > According to doc, LDPR_PREVAILING_DEF is the definition accessed > outside > > of LTO IR as opposed to LDPR_PREVAILING_DEF_IRONLY. So I think it > should > > be externally visible. In my test file, bar2 is the similar as > eliminate_me > > in your testcase. It is correctly eliminated with -fwhole-program. > > Oh, I see. (where's that doc btw?) Actually comments in plugin-api.h in binutils enum ld_plugin_symbol_resolution { LDPR_UNKNOWN = 0, /* Symbol is still undefined at this point. */ LDPR_UNDEF, /* This is the prevailing definition of the symbol, with references from regular object code. */ LDPR_PREVAILING_DEF, /* This is the prevailing definition of the symbol, with no references from regular objects. It is only referenced from IR code. */ LDPR_PREVAILING_DEF_IRONLY, /* This definition was pre-empted by a definition in a regular object file. */ LDPR_PREEMPTED_REG, /* This definition was pre-empted by a definition in another IR file. */ LDPR_PREEMPTED_IR, /* This symbol was resolved by a definition in another IR file. */ LDPR_RESOLVED_IR, /* This symbol was resolved by a definition in a regular object linked into the main executable. */ LDPR_RESOLVED_EXEC, /* This symbol was resolved by a definition in a shared object. */ LDPR_RESOLVED_DYN }; > > I guess the internal resolver should then be updated to use > IRONLY and applying external visibility should be done at > the end of lto_symtab_merge_decls_1 then. > > Honza - what's the cgraph/varpool way of marking sth externally > visible? > > Richard. > > > > >> > >> 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. > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-09 15:13 ` Richard Guenther 2010-06-09 15:25 ` Bingfeng Mei @ 2010-06-09 15:32 ` Bingfeng Mei 2010-06-09 15:48 ` Richard Guenther 1 sibling, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-09 15:32 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches 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. 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-09 15:32 ` Bingfeng Mei @ 2010-06-09 15:48 ` Richard Guenther 2010-06-10 10:30 ` Bingfeng Mei 2010-06-14 9:17 ` Bingfeng Mei 0 siblings, 2 replies; 27+ messages in thread From: Richard Guenther @ 2010-06-09 15:48 UTC (permalink / raw) To: Bingfeng Mei; +Cc: gcc-patches 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. > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-09 15:48 ` Richard Guenther @ 2010-06-10 10:30 ` Bingfeng Mei 2010-06-10 10:35 ` Richard Guenther 2010-06-14 9:17 ` Bingfeng Mei 1 sibling, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-10 10:30 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches Richard, I tried to set externally_visible flags directly for cgraph node & varbool node. Unfortunately, these flags seem to be set and used elsewhere too. The values set in lto-symtab.c cannot be reliably passed to ipa.c (function_and_variable_visibility), where the flags are actually re-computed. if (cgraph_externally_visible_p (node, whole_program)) { gcc_assert (!node->global.inlined_to); node->local.externally_visible = true; } else node->local.externally_visible = false; The flag for bar2 function is true before this piece of code even I doesn't set it in lto-symtab.c. For now, I think attribute is still cleaner solution though not cheap. The following is the updated patch. Cheers, Bingfeng 2010-06-10 Bingfeng Mei <bmei@broadcom.com> * lto-symbtab.c (lto_symtab_merge_decls_1): Add externally_visible attribute for declaration of LDPR_PREVAILING_DEF when compiling with -fwhole-program (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY for internal resolver Index: lto-symtab.c =================================================================== --- lto-symtab.c (revision 160463) +++ lto-symtab.c (working copy) @@ -530,11 +530,15 @@ return; found: - if (TREE_CODE (prevailing->decl) == VAR_DECL - && TREE_READONLY (prevailing->decl)) - prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; - else - prevailing->resolution = LDPR_PREVAILING_DEF; + /* 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 */ + prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; } /* Merge all decls in the symbol table chain to the prevailing decl and @@ -698,6 +702,18 @@ && TREE_CODE (prevailing->decl) != VAR_DECL) prevailing->next = NULL; + + /* Add externally_visible attribute for declaration of LDPR_PREVAILING_DEF */ + if (prevailing->resolution == LDPR_PREVAILING_DEF && flag_whole_program) + { + if (!lookup_attribute ("externally_visible", + DECL_ATTRIBUTES (prevailing->decl))) + { + DECL_ATTRIBUTES (prevailing->decl) + = tree_cons (get_identifier ("externally_visible"), NULL_TREE, + DECL_ATTRIBUTES (prevailing->decl)); + } + } return 1; } > -----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. > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-10 10:30 ` Bingfeng Mei @ 2010-06-10 10:35 ` Richard Guenther 2010-06-10 15:31 ` Bingfeng Mei 0 siblings, 1 reply; 27+ messages in thread From: Richard Guenther @ 2010-06-10 10:35 UTC (permalink / raw) To: Bingfeng Mei; +Cc: gcc-patches, Jan Hubicka On Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com> wrote: > Richard, > > I tried to set externally_visible flags directly for cgraph node & varbool node. > Unfortunately, these flags seem to be set and used elsewhere too. The values > set in lto-symtab.c cannot be reliably passed to ipa.c (function_and_variable_visibility), > where the flags are actually re-computed. > > if (cgraph_externally_visible_p (node, whole_program)) > { > gcc_assert (!node->global.inlined_to); > node->local.externally_visible = true; > } > else > node->local.externally_visible = false; > > The flag for bar2 function is true before this piece of code even I doesn't set > it in lto-symtab.c. I believe this is an error - Honza knows the code and will probably comment. > For now, I think attribute is still cleaner solution though > not cheap. The following is the updated patch. Thanks - I believe this is valuable but want to hear comments from Honza as we shouldn't need to use attributes here. Richard. > Cheers, > Bingfeng > > 2010-06-10 Bingfeng Mei <bmei@broadcom.com> > > * lto-symbtab.c (lto_symtab_merge_decls_1): Add externally_visible > attribute for declaration of LDPR_PREVAILING_DEF when compiling with > -fwhole-program > (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY for > internal resolver > > Index: lto-symtab.c > =================================================================== > --- lto-symtab.c (revision 160463) > +++ lto-symtab.c (working copy) > @@ -530,11 +530,15 @@ > return; > > found: > - if (TREE_CODE (prevailing->decl) == VAR_DECL > - && TREE_READONLY (prevailing->decl)) > - prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; > - else > - prevailing->resolution = LDPR_PREVAILING_DEF; > + /* 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 */ > + prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; > } > > /* Merge all decls in the symbol table chain to the prevailing decl and > @@ -698,6 +702,18 @@ > && TREE_CODE (prevailing->decl) != VAR_DECL) > prevailing->next = NULL; > > + > + /* Add externally_visible attribute for declaration of LDPR_PREVAILING_DEF */ > + if (prevailing->resolution == LDPR_PREVAILING_DEF && flag_whole_program) > + { > + if (!lookup_attribute ("externally_visible", > + DECL_ATTRIBUTES (prevailing->decl))) > + { > + DECL_ATTRIBUTES (prevailing->decl) > + = tree_cons (get_identifier ("externally_visible"), NULL_TREE, > + DECL_ATTRIBUTES (prevailing->decl)); > + } > + } > return 1; > } > >> -----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. >> > >> > >> > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-10 10:35 ` Richard Guenther @ 2010-06-10 15:31 ` Bingfeng Mei 2010-06-10 17:07 ` Richard Guenther 0 siblings, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-10 15:31 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches, Jan Hubicka Richard, I figured out what happen with externally_visible flags in cgraph node/vnode. In ipa.c (function_and_varialbe_visibility), the externally_visible flags are set to true when compile with -flto because cgraph_externally_visible_p always returns true as whole_program is false (regardless -fwhole-program option is set or not, which is always ignored in the first phase of LTO compilation). if (cgraph_externally_visible_p (node, whole_program)) { gcc_assert (!node->global.inlined_to); node->local.externally_visible = true; } Then the flags are packed/unpacked from LTO byte code. That's why the externally_visible flag is set to true for "bar2" function before executing my patch. Another issue is following code (your patch on 22/5). The vvvvvv variable in my example is going to be resolved by internal resolver as LDPR_PREVAILING_DEF_IRONLY instead of LDPR_PREVAILING_DEF by resolution file. Could you Elaborate what is the exact issue here? /* The LTO plugin for gold doesn't handle common symbols properly. Let us choose manually. */ if (DECL_COMMON (e->decl)) e->resolution = LDPR_UNKNOWN; Thanks, Bingfeng > -----Original Message----- > From: Richard Guenther [mailto:richard.guenther@gmail.com] > Sent: 10 June 2010 11:12 > 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 Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com> > wrote: > > Richard, > > > > I tried to set externally_visible flags directly for cgraph node & > varbool node. > > Unfortunately, these flags seem to be set and used elsewhere too. The > values > > set in lto-symtab.c cannot be reliably passed to ipa.c > (function_and_variable_visibility), > > where the flags are actually re-computed. > > > > if (cgraph_externally_visible_p (node, whole_program)) > > { > > gcc_assert (!node->global.inlined_to); > > node->local.externally_visible = true; > > } > > else > > node->local.externally_visible = false; > > > > The flag for bar2 function is true before this piece of code even I > doesn't set > > it in lto-symtab.c. > > I believe this is an error - Honza knows the code and will probably > comment. > > > For now, I think attribute is still cleaner solution though > > not cheap. The following is the updated patch. > > Thanks - I believe this is valuable but want to hear comments > from Honza as we shouldn't need to use attributes here. > > Richard. > > > Cheers, > > Bingfeng > > > > 2010-06-10 Bingfeng Mei <bmei@broadcom.com> > > > > * lto-symbtab.c (lto_symtab_merge_decls_1): Add > externally_visible > > attribute for declaration of LDPR_PREVAILING_DEF when > compiling with > > -fwhole-program > > (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY > for > > internal resolver > > > > Index: lto-symtab.c > > =================================================================== > > --- lto-symtab.c (revision 160463) > > +++ lto-symtab.c (working copy) > > @@ -530,11 +530,15 @@ > > return; > > > > found: > > - if (TREE_CODE (prevailing->decl) == VAR_DECL > > - && TREE_READONLY (prevailing->decl)) > > - prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; > > - else > > - prevailing->resolution = LDPR_PREVAILING_DEF; > > + /* 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 */ > > + prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; > > } > > > > /* Merge all decls in the symbol table chain to the prevailing decl > and > > @@ -698,6 +702,18 @@ > > && TREE_CODE (prevailing->decl) != VAR_DECL) > > prevailing->next = NULL; > > > > + > > + /* Add externally_visible attribute for declaration of > LDPR_PREVAILING_DEF */ > > + if (prevailing->resolution == LDPR_PREVAILING_DEF && > flag_whole_program) > > + { > > + if (!lookup_attribute ("externally_visible", > > + DECL_ATTRIBUTES (prevailing->decl))) > > + { > > + DECL_ATTRIBUTES (prevailing->decl) > > + = tree_cons (get_identifier ("externally_visible"), > NULL_TREE, > > + DECL_ATTRIBUTES (prevailing->decl)); > > + } > > + } > > return 1; > > } > > > >> -----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. > >> > > >> > > >> > > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-10 15:31 ` Bingfeng Mei @ 2010-06-10 17:07 ` Richard Guenther 2010-06-10 17:10 ` Bingfeng Mei 0 siblings, 1 reply; 27+ messages in thread From: Richard Guenther @ 2010-06-10 17:07 UTC (permalink / raw) To: Bingfeng Mei; +Cc: gcc-patches, Jan Hubicka On Thu, Jun 10, 2010 at 4:51 PM, Bingfeng Mei <bmei@broadcom.com> wrote: > Richard, > I figured out what happen with externally_visible flags in cgraph node/vnode. > > In ipa.c (function_and_varialbe_visibility), the externally_visible flags are set to > true when compile with -flto because cgraph_externally_visible_p always returns true as > whole_program is false (regardless -fwhole-program option is set or not, which is > always ignored in the first phase of LTO compilation). > > if (cgraph_externally_visible_p (node, whole_program)) > { > gcc_assert (!node->global.inlined_to); > node->local.externally_visible = true; > } > > Then the flags are packed/unpacked from LTO byte code. That's why the externally_visible > flag is set to true for "bar2" function before executing my patch. > > > > Another issue is following code (your patch on 22/5). The vvvvvv > variable in my example is going to be resolved by internal > resolver as LDPR_PREVAILING_DEF_IRONLY instead of > LDPR_PREVAILING_DEF by resolution file. Could you > Elaborate what is the exact issue here? Gold does not keep track of which symbol from which object file it will end up using for commons but instead simply picks the first one. We rely on the fact that we get the one with the largest size - with gold we even can end up with a non-prevailing one. And there's some version of gold which doesn't resolve commons at all. So we need to run through our own resolution code here. An example is extern int i; -- int i; where gold can claim that 'extern int i' was prevailing. > /* The LTO plugin for gold doesn't handle common symbols > properly. Let us choose manually. */ > if (DECL_COMMON (e->decl)) > e->resolution = LDPR_UNKNOWN; > > Thanks, > Bingfeng >> -----Original Message----- >> From: Richard Guenther [mailto:richard.guenther@gmail.com] >> Sent: 10 June 2010 11:12 >> 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 Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com> >> wrote: >> > Richard, >> > >> > I tried to set externally_visible flags directly for cgraph node & >> varbool node. >> > Unfortunately, these flags seem to be set and used elsewhere too. The >> values >> > set in lto-symtab.c cannot be reliably passed to ipa.c >> (function_and_variable_visibility), >> > where the flags are actually re-computed. >> > >> > if (cgraph_externally_visible_p (node, whole_program)) >> > { >> > gcc_assert (!node->global.inlined_to); >> > node->local.externally_visible = true; >> > } >> > else >> > node->local.externally_visible = false; >> > >> > The flag for bar2 function is true before this piece of code even I >> doesn't set >> > it in lto-symtab.c. >> >> I believe this is an error - Honza knows the code and will probably >> comment. >> >> > For now, I think attribute is still cleaner solution though >> > not cheap. The following is the updated patch. >> >> Thanks - I believe this is valuable but want to hear comments >> from Honza as we shouldn't need to use attributes here. >> >> Richard. >> >> > Cheers, >> > Bingfeng >> > >> > 2010-06-10 Bingfeng Mei <bmei@broadcom.com> >> > >> > * lto-symbtab.c (lto_symtab_merge_decls_1): Add >> externally_visible >> > attribute for declaration of LDPR_PREVAILING_DEF when >> compiling with >> > -fwhole-program >> > (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY >> for >> > internal resolver >> > >> > Index: lto-symtab.c >> > =================================================================== >> > --- lto-symtab.c (revision 160463) >> > +++ lto-symtab.c (working copy) >> > @@ -530,11 +530,15 @@ >> > return; >> > >> > found: >> > - if (TREE_CODE (prevailing->decl) == VAR_DECL >> > - && TREE_READONLY (prevailing->decl)) >> > - prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; >> > - else >> > - prevailing->resolution = LDPR_PREVAILING_DEF; >> > + /* 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 */ >> > + prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; >> > } >> > >> > /* Merge all decls in the symbol table chain to the prevailing decl >> and >> > @@ -698,6 +702,18 @@ >> > && TREE_CODE (prevailing->decl) != VAR_DECL) >> > prevailing->next = NULL; >> > >> > + >> > + /* Add externally_visible attribute for declaration of >> LDPR_PREVAILING_DEF */ >> > + if (prevailing->resolution == LDPR_PREVAILING_DEF && >> flag_whole_program) >> > + { >> > + if (!lookup_attribute ("externally_visible", >> > + DECL_ATTRIBUTES (prevailing->decl))) >> > + { >> > + DECL_ATTRIBUTES (prevailing->decl) >> > + = tree_cons (get_identifier ("externally_visible"), >> NULL_TREE, >> > + DECL_ATTRIBUTES (prevailing->decl)); >> > + } >> > + } >> > return 1; >> > } >> > >> >> -----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. >> >> > >> >> > >> >> > >> > >> > >> > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-10 17:07 ` Richard Guenther @ 2010-06-10 17:10 ` Bingfeng Mei 2010-06-11 9:34 ` Richard Guenther 0 siblings, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-10 17:10 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches, Jan Hubicka Actually, I got correct results from gold x.c extern int i; y.c int i; ~/work/install-x86/bin/gcc y.o x.o -O2 -fuse-linker-plugin -o f -flto -fwhole-program -save-temps x.res: 2 y.o 1 78 PREVAILING_DEF_IRONLY i x.o 0 ~/work/install-x86/bin/gcc x.o y.o -O2 -fuse-linker-plugin -o f -flto -fwhole-program -save-temps y.res 2 x.o 0 y.o 1 78 PREVAILING_DEF_IRONLY i I am using binutils-2.20-51. Bingfeng > -----Original Message----- > From: Richard Guenther [mailto:richard.guenther@gmail.com] > Sent: 10 June 2010 17:12 > 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 Thu, Jun 10, 2010 at 4:51 PM, Bingfeng Mei <bmei@broadcom.com> wrote: > > Richard, > > I figured out what happen with externally_visible flags in cgraph > node/vnode. > > > > In ipa.c (function_and_varialbe_visibility), the externally_visible > flags are set to > > true when compile with -flto because cgraph_externally_visible_p > always returns true as > > whole_program is false (regardless -fwhole-program option is set or > not, which is > > always ignored in the first phase of LTO compilation). > > > > if (cgraph_externally_visible_p (node, whole_program)) > > { > > gcc_assert (!node->global.inlined_to); > > node->local.externally_visible = true; > > } > > > > Then the flags are packed/unpacked from LTO byte code. That's why the > externally_visible > > flag is set to true for "bar2" function before executing my patch. > > > > > > > > Another issue is following code (your patch on 22/5). The vvvvvv > > variable in my example is going to be resolved by internal > > resolver as LDPR_PREVAILING_DEF_IRONLY instead of > > LDPR_PREVAILING_DEF by resolution file. Could you > > Elaborate what is the exact issue here? > > Gold does not keep track of which symbol from which object > file it will end up using for commons but instead simply picks > the first one. We rely on the fact that we get the one with > the largest size - with gold we even can end up with > a non-prevailing one. And there's some version of gold which doesn't > resolve commons at all. > > So we need to run through our own resolution code here. > > An example is > > extern int i; > -- > int i; > > where gold can claim that 'extern int i' was prevailing. > > > /* The LTO plugin for gold doesn't handle common symbols > > properly. Let us choose manually. */ > > if (DECL_COMMON (e->decl)) > > e->resolution = LDPR_UNKNOWN; > > > > Thanks, > > Bingfeng > >> -----Original Message----- > >> From: Richard Guenther [mailto:richard.guenther@gmail.com] > >> Sent: 10 June 2010 11:12 > >> 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 Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com> > >> wrote: > >> > Richard, > >> > > >> > I tried to set externally_visible flags directly for cgraph node & > >> varbool node. > >> > Unfortunately, these flags seem to be set and used elsewhere too. > The > >> values > >> > set in lto-symtab.c cannot be reliably passed to ipa.c > >> (function_and_variable_visibility), > >> > where the flags are actually re-computed. > >> > > >> > if (cgraph_externally_visible_p (node, whole_program)) > >> > { > >> > gcc_assert (!node->global.inlined_to); > >> > node->local.externally_visible = true; > >> > } > >> > else > >> > node->local.externally_visible = false; > >> > > >> > The flag for bar2 function is true before this piece of code even > I > >> doesn't set > >> > it in lto-symtab.c. > >> > >> I believe this is an error - Honza knows the code and will probably > >> comment. > >> > >> > For now, I think attribute is still cleaner solution though > >> > not cheap. The following is the updated patch. > >> > >> Thanks - I believe this is valuable but want to hear comments > >> from Honza as we shouldn't need to use attributes here. > >> > >> Richard. > >> > >> > Cheers, > >> > Bingfeng > >> > > >> > 2010-06-10 Bingfeng Mei <bmei@broadcom.com> > >> > > >> > * lto-symbtab.c (lto_symtab_merge_decls_1): Add > >> externally_visible > >> > attribute for declaration of LDPR_PREVAILING_DEF when > >> compiling with > >> > -fwhole-program > >> > (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY > >> for > >> > internal resolver > >> > > >> > Index: lto-symtab.c > >> > > =================================================================== > >> > --- lto-symtab.c (revision 160463) > >> > +++ lto-symtab.c (working copy) > >> > @@ -530,11 +530,15 @@ > >> > return; > >> > > >> > found: > >> > - if (TREE_CODE (prevailing->decl) == VAR_DECL > >> > - && TREE_READONLY (prevailing->decl)) > >> > - prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; > >> > - else > >> > - prevailing->resolution = LDPR_PREVAILING_DEF; > >> > + /* 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 */ > >> > + prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; > >> > } > >> > > >> > /* Merge all decls in the symbol table chain to the prevailing > decl > >> and > >> > @@ -698,6 +702,18 @@ > >> > && TREE_CODE (prevailing->decl) != VAR_DECL) > >> > prevailing->next = NULL; > >> > > >> > + > >> > + /* Add externally_visible attribute for declaration of > >> LDPR_PREVAILING_DEF */ > >> > + if (prevailing->resolution == LDPR_PREVAILING_DEF && > >> flag_whole_program) > >> > + { > >> > + if (!lookup_attribute ("externally_visible", > >> > + DECL_ATTRIBUTES (prevailing->decl))) > >> > + { > >> > + DECL_ATTRIBUTES (prevailing->decl) > >> > + = tree_cons (get_identifier ("externally_visible"), > >> NULL_TREE, > >> > + DECL_ATTRIBUTES (prevailing->decl)); > >> > + } > >> > + } > >> > return 1; > >> > } > >> > > >> >> -----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. > >> >> > > >> >> > > >> >> > > >> > > >> > > >> > > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-10 17:10 ` Bingfeng Mei @ 2010-06-11 9:34 ` Richard Guenther 2010-06-14 18:58 ` Cary Coutant 0 siblings, 1 reply; 27+ messages in thread From: Richard Guenther @ 2010-06-11 9:34 UTC (permalink / raw) To: Bingfeng Mei; +Cc: gcc-patches, Jan Hubicka On Thu, Jun 10, 2010 at 6:23 PM, Bingfeng Mei <bmei@broadcom.com> wrote: > Actually, I got correct results from gold > x.c > extern int i; > > y.c > int i; > > ~/work/install-x86/bin/gcc y.o x.o -O2 -fuse-linker-plugin -o f -flto -fwhole-program -save-temps > x.res: > 2 > y.o 1 > 78 PREVAILING_DEF_IRONLY i > x.o 0 > > ~/work/install-x86/bin/gcc x.o y.o -O2 -fuse-linker-plugin -o f -flto -fwhole-program -save-temps > y.res > 2 > x.o 0 > y.o 1 > 78 PREVAILING_DEF_IRONLY i > > > I am using binutils-2.20-51. Yes, the above was a bug that was actually fixed. The issue that it chooses a random fortran common block and not the largest one still prevails. We might consider reverting the change on the trunk (it's certainly safe on the branch and makes more things work there). But it would also be nice for either gold or the linker-plugin being fixed for the size issue. (I think you run into the issue when building SPEC 2k6, you might also find a closed bugzilla that reports it) Richard. > Bingfeng >> -----Original Message----- >> From: Richard Guenther [mailto:richard.guenther@gmail.com] >> Sent: 10 June 2010 17:12 >> 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 Thu, Jun 10, 2010 at 4:51 PM, Bingfeng Mei <bmei@broadcom.com> wrote: >> > Richard, >> > I figured out what happen with externally_visible flags in cgraph >> node/vnode. >> > >> > In ipa.c (function_and_varialbe_visibility), the externally_visible >> flags are set to >> > true when compile with -flto because cgraph_externally_visible_p >> always returns true as >> > whole_program is false (regardless -fwhole-program option is set or >> not, which is >> > always ignored in the first phase of LTO compilation). >> > >> > if (cgraph_externally_visible_p (node, whole_program)) >> > { >> > gcc_assert (!node->global.inlined_to); >> > node->local.externally_visible = true; >> > } >> > >> > Then the flags are packed/unpacked from LTO byte code. That's why the >> externally_visible >> > flag is set to true for "bar2" function before executing my patch. >> > >> > >> > >> > Another issue is following code (your patch on 22/5). The vvvvvv >> > variable in my example is going to be resolved by internal >> > resolver as LDPR_PREVAILING_DEF_IRONLY instead of >> > LDPR_PREVAILING_DEF by resolution file. Could you >> > Elaborate what is the exact issue here? >> >> Gold does not keep track of which symbol from which object >> file it will end up using for commons but instead simply picks >> the first one. We rely on the fact that we get the one with >> the largest size - with gold we even can end up with >> a non-prevailing one. And there's some version of gold which doesn't >> resolve commons at all. >> >> So we need to run through our own resolution code here. >> >> An example is >> >> extern int i; >> -- >> int i; >> >> where gold can claim that 'extern int i' was prevailing. >> >> > /* The LTO plugin for gold doesn't handle common symbols >> > properly. Let us choose manually. */ >> > if (DECL_COMMON (e->decl)) >> > e->resolution = LDPR_UNKNOWN; >> > >> > Thanks, >> > Bingfeng >> >> -----Original Message----- >> >> From: Richard Guenther [mailto:richard.guenther@gmail.com] >> >> Sent: 10 June 2010 11:12 >> >> 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 Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com> >> >> wrote: >> >> > Richard, >> >> > >> >> > I tried to set externally_visible flags directly for cgraph node & >> >> varbool node. >> >> > Unfortunately, these flags seem to be set and used elsewhere too. >> The >> >> values >> >> > set in lto-symtab.c cannot be reliably passed to ipa.c >> >> (function_and_variable_visibility), >> >> > where the flags are actually re-computed. >> >> > >> >> > if (cgraph_externally_visible_p (node, whole_program)) >> >> > { >> >> > gcc_assert (!node->global.inlined_to); >> >> > node->local.externally_visible = true; >> >> > } >> >> > else >> >> > node->local.externally_visible = false; >> >> > >> >> > The flag for bar2 function is true before this piece of code even >> I >> >> doesn't set >> >> > it in lto-symtab.c. >> >> >> >> I believe this is an error - Honza knows the code and will probably >> >> comment. >> >> >> >> > For now, I think attribute is still cleaner solution though >> >> > not cheap. The following is the updated patch. >> >> >> >> Thanks - I believe this is valuable but want to hear comments >> >> from Honza as we shouldn't need to use attributes here. >> >> >> >> Richard. >> >> >> >> > Cheers, >> >> > Bingfeng >> >> > >> >> > 2010-06-10 Bingfeng Mei <bmei@broadcom.com> >> >> > >> >> > * lto-symbtab.c (lto_symtab_merge_decls_1): Add >> >> externally_visible >> >> > attribute for declaration of LDPR_PREVAILING_DEF when >> >> compiling with >> >> > -fwhole-program >> >> > (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY >> >> for >> >> > internal resolver >> >> > >> >> > Index: lto-symtab.c >> >> > >> =================================================================== >> >> > --- lto-symtab.c (revision 160463) >> >> > +++ lto-symtab.c (working copy) >> >> > @@ -530,11 +530,15 @@ >> >> > return; >> >> > >> >> > found: >> >> > - if (TREE_CODE (prevailing->decl) == VAR_DECL >> >> > - && TREE_READONLY (prevailing->decl)) >> >> > - prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; >> >> > - else >> >> > - prevailing->resolution = LDPR_PREVAILING_DEF; >> >> > + /* 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 */ >> >> > + prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY; >> >> > } >> >> > >> >> > /* Merge all decls in the symbol table chain to the prevailing >> decl >> >> and >> >> > @@ -698,6 +702,18 @@ >> >> > && TREE_CODE (prevailing->decl) != VAR_DECL) >> >> > prevailing->next = NULL; >> >> > >> >> > + >> >> > + /* Add externally_visible attribute for declaration of >> >> LDPR_PREVAILING_DEF */ >> >> > + if (prevailing->resolution == LDPR_PREVAILING_DEF && >> >> flag_whole_program) >> >> > + { >> >> > + if (!lookup_attribute ("externally_visible", >> >> > + DECL_ATTRIBUTES (prevailing->decl))) >> >> > + { >> >> > + DECL_ATTRIBUTES (prevailing->decl) >> >> > + = tree_cons (get_identifier ("externally_visible"), >> >> NULL_TREE, >> >> > + DECL_ATTRIBUTES (prevailing->decl)); >> >> > + } >> >> > + } >> >> > return 1; >> >> > } >> >> > >> >> >> -----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. >> >> >> > >> >> >> > >> >> >> > >> >> > >> >> > >> >> > >> > >> > >> > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-11 9:34 ` Richard Guenther @ 2010-06-14 18:58 ` Cary Coutant 2010-06-14 20:03 ` Richard Guenther 0 siblings, 1 reply; 27+ messages in thread From: Cary Coutant @ 2010-06-14 18:58 UTC (permalink / raw) To: Richard Guenther; +Cc: Bingfeng Mei, gcc-patches, Jan Hubicka > Yes, the above was a bug that was actually fixed. The issue that > it chooses a random fortran common block and not the largest > one still prevails. > > We might consider reverting the change on the trunk (it's certainly > safe on the branch and makes more things work there). But it > would also be nice for either gold or the linker-plugin being fixed > for the size issue. (I think you run into the issue when building > SPEC 2k6, you might also find a closed bugzilla that reports it) Yes, gold merges common symbols by keeping the first and simply increasing its size to the maximum of all subsequent ones. By the time the plugin asks for symbol resolution information, there's no record of which one was actually the largest. I'll work on a fix for this. Is this in fact what PR 44149 is about? -cary ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-14 18:58 ` Cary Coutant @ 2010-06-14 20:03 ` Richard Guenther 0 siblings, 0 replies; 27+ messages in thread From: Richard Guenther @ 2010-06-14 20:03 UTC (permalink / raw) To: Cary Coutant; +Cc: Bingfeng Mei, gcc-patches, Jan Hubicka On Mon, Jun 14, 2010 at 8:43 PM, Cary Coutant <ccoutant@google.com> wrote: >> Yes, the above was a bug that was actually fixed. The issue that >> it chooses a random fortran common block and not the largest >> one still prevails. >> >> We might consider reverting the change on the trunk (it's certainly >> safe on the branch and makes more things work there). But it >> would also be nice for either gold or the linker-plugin being fixed >> for the size issue. (I think you run into the issue when building >> SPEC 2k6, you might also find a closed bugzilla that reports it) > > Yes, gold merges common symbols by keeping the first and simply > increasing its size to the maximum of all subsequent ones. By the time > the plugin asks for symbol resolution information, there's no record > of which one was actually the largest. I'll work on a fix for this. > > Is this in fact what PR 44149 is about? Yes. In that PR the linker-plugin reports a file with only a DECL_EXTERNAL symbol as providing the prevailing one even. Richard. > -cary > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-09 15:48 ` Richard Guenther 2010-06-10 10:30 ` Bingfeng Mei @ 2010-06-14 9:17 ` Bingfeng Mei 2010-06-14 10:10 ` Richard Guenther 1 sibling, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-14 9:17 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches, Jan Hubicka 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 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)); > -----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. > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-14 9:17 ` Bingfeng Mei @ 2010-06-14 10:10 ` Richard Guenther 2010-06-14 11:33 ` Bingfeng Mei ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Richard Guenther @ 2010-06-14 10:10 UTC (permalink / raw) To: Bingfeng Mei; +Cc: gcc-patches, Jan Hubicka 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. >> > >> > >> > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-14 10:10 ` Richard Guenther @ 2010-06-14 11:33 ` Bingfeng Mei 2010-06-14 15:36 ` Bingfeng Mei 2010-06-18 9:28 ` PING: " Bingfeng Mei 2 siblings, 0 replies; 27+ messages in thread From: Bingfeng Mei @ 2010-06-14 11:33 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches, Jan Hubicka [-- Attachment #1: Type: text/plain, Size: 12488 bytes --] 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. > >> > > >> > > >> > > > > > > > [-- Attachment #2: b.c --] [-- Type: text/plain, Size: 171 bytes --] #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; } [-- Attachment #3: a.c --] [-- Type: text/plain, Size: 417 bytes --] #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; } ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-14 10:10 ` Richard Guenther 2010-06-14 11:33 ` Bingfeng Mei @ 2010-06-14 15:36 ` Bingfeng Mei 2010-06-18 9:28 ` PING: " Bingfeng Mei 2 siblings, 0 replies; 27+ messages in thread From: Bingfeng Mei @ 2010-06-14 15:36 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Richard Guenther [-- Attachment #1: Type: text/plain, Size: 12098 bytes --] 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. > >> > > >> > > >> > > > > > > > [-- Attachment #2: patch --] [-- Type: application/octet-stream, Size: 5569 bytes --] Index: doc/extend.texi =================================================================== --- doc/extend.texi (revision 160529) +++ doc/extend.texi (working copy) @@ -2296,7 +2296,7 @@ @cindex @code{externally_visible} attribute. This attribute, attached to a global variable or function, nullifies the effect of the @option{-fwhole-program} command-line option, so the -object remains visible outside the current compilation unit. +object remains visible outside the current compilation unit. If @option{-fwhole-program} is used together with @option{-flto} and @command{gold} is used as the linker plugin, @code{externally_visible} attributes are automatically added to functions (not variable yet due to a current @command{gold} issue) that are accessed outside of LTO objects according to resolution file produced by @command{gold}. For other linkers that cannot generate resolution file, explicit @code{externally_visible} attributes are still necessary. @item far @cindex functions which handle memory bank switching Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 160529) +++ doc/invoke.texi (working copy) @@ -7294,7 +7294,7 @@ Assume that the current compilation unit represents the whole program being compiled. All public functions and variables with the exception of @code{main} and those merged by attribute @code{externally_visible} become static functions -and in effect are optimized more aggressively by interprocedural optimizers. +and in effect are optimized more aggressively by interprocedural optimizers. If @command{gold} is used as the linker plugin, @code{externally_visible} attributes are automatically added to functions (not variable yet due to a current @command{gold} issue) that are accessed outside of LTO objects according to resolution file produced by @command{gold}. For other linkers that cannot generate resolution file, explicit @code{externally_visible} attributes are still necessary. While this option is equivalent to proper use of the @code{static} keyword for programs consisting of a single file, in combination with option @option{-combine}, @option{-flto} or @option{-fwhopr} this flag can be used to Index: lto-symtab.c =================================================================== --- lto-symtab.c (revision 160529) +++ lto-symtab.c (working copy) @@ -530,11 +530,20 @@ 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 +707,24 @@ && TREE_CODE (prevailing->decl) != VAR_DECL) prevailing->next = NULL; + /* Set externally_visible flags 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)); ^ permalink raw reply [flat|nested] 27+ messages in thread
* PING: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-14 10:10 ` Richard Guenther 2010-06-14 11:33 ` Bingfeng Mei 2010-06-14 15:36 ` Bingfeng Mei @ 2010-06-18 9:28 ` Bingfeng Mei 2010-06-18 16:46 ` Jan Hubicka 2 siblings, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-18 9:28 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Richard Guenther [-- Attachment #1: Type: text/plain, Size: 12105 bytes --] 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. > >> > > >> > > >> > > > > > > > [-- Attachment #2: patch --] [-- Type: application/octet-stream, Size: 5569 bytes --] Index: doc/extend.texi =================================================================== --- doc/extend.texi (revision 160529) +++ doc/extend.texi (working copy) @@ -2296,7 +2296,7 @@ @cindex @code{externally_visible} attribute. This attribute, attached to a global variable or function, nullifies the effect of the @option{-fwhole-program} command-line option, so the -object remains visible outside the current compilation unit. +object remains visible outside the current compilation unit. If @option{-fwhole-program} is used together with @option{-flto} and @command{gold} is used as the linker plugin, @code{externally_visible} attributes are automatically added to functions (not variable yet due to a current @command{gold} issue) that are accessed outside of LTO objects according to resolution file produced by @command{gold}. For other linkers that cannot generate resolution file, explicit @code{externally_visible} attributes are still necessary. @item far @cindex functions which handle memory bank switching Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 160529) +++ doc/invoke.texi (working copy) @@ -7294,7 +7294,7 @@ Assume that the current compilation unit represents the whole program being compiled. All public functions and variables with the exception of @code{main} and those merged by attribute @code{externally_visible} become static functions -and in effect are optimized more aggressively by interprocedural optimizers. +and in effect are optimized more aggressively by interprocedural optimizers. If @command{gold} is used as the linker plugin, @code{externally_visible} attributes are automatically added to functions (not variable yet due to a current @command{gold} issue) that are accessed outside of LTO objects according to resolution file produced by @command{gold}. For other linkers that cannot generate resolution file, explicit @code{externally_visible} attributes are still necessary. While this option is equivalent to proper use of the @code{static} keyword for programs consisting of a single file, in combination with option @option{-combine}, @option{-flto} or @option{-fwhopr} this flag can be used to Index: lto-symtab.c =================================================================== --- lto-symtab.c (revision 160529) +++ lto-symtab.c (working copy) @@ -530,11 +530,20 @@ 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 +707,24 @@ && TREE_CODE (prevailing->decl) != VAR_DECL) prevailing->next = NULL; + /* Set externally_visible flags 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)); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: PING: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-18 9:28 ` PING: " Bingfeng Mei @ 2010-06-18 16:46 ` Jan Hubicka 2010-06-18 16:56 ` Bingfeng Mei 0 siblings, 1 reply; 27+ messages in thread From: Jan Hubicka @ 2010-06-18 16:46 UTC (permalink / raw) To: Bingfeng Mei; +Cc: Jan Hubicka, gcc-patches, Richard Guenther 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; This is not correct: the externally visible attribute is computed twice; once in pass_ipa_function_and_variable_visibility and second time in pass_ipa_whole_program_visibility. This is so to allow output from early IPA passes and early local optimization to be used by both at the compile time and link time. So variables/functions are externally visible per their visiblities first and then in pass_ipa_whole_program_visibility they might become static. This is why the conditional sets externally_visible to false. Not doing so you effectively disbale -fwhole-program. We might get around by not streaming externally_visible flag and not clearling it when in LTO mode (since it would not be set for other reasons) but this is bit fragile. We already have flag used_from_other_partition, this situation is similar, just it is not used from other partition but externally. Perhaps we can just add specific flag for this purpose and update cgraph_externally_visible_p to test for it? Honza ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: PING: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-18 16:46 ` Jan Hubicka @ 2010-06-18 16:56 ` Bingfeng Mei 2010-06-18 17:25 ` Jan Hubicka 0 siblings, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-18 16:56 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Richard Guenther > -----Original Message----- > From: Jan Hubicka [mailto:hubicka@ucw.cz] > Sent: 18 June 2010 15:49 > To: Bingfeng Mei > Cc: Jan Hubicka; gcc-patches@gcc.gnu.org; Richard Guenther > Subject: Re: PING: [PATCH, LTO] add externally_visible attribute when > necessary with -fwhole-program and resolution file. > > 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; > > This is not correct: the externally visible attribute is computed twice; > once > in pass_ipa_function_and_variable_visibility and second time in > pass_ipa_whole_program_visibility. > > This is so to allow output from early IPA passes and early local > optimization > to be used by both at the compile time and link time. So > variables/functions > are externally visible per their visiblities first and then in > pass_ipa_whole_program_visibility they might become static. > > This is why the conditional sets externally_visible to false. Not > doing so you > effectively disbale -fwhole-program. Actually, the externally_visible flags will always be set to either true or false with -fwhole-program due to following code. The unpacked values from LTO bytecode won't be passed though to effectively disable -fwhole-program. + /* Set externally_visible flags 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; + } + } The difference from original code is that when no -fwhole-program specified, the true value of externally_visible (from pass_ipa_function_and_variable_visibility) will be passed on and not conditionally set to false in pass_ipa_whole_program_visibility. But does that matter? Cgraph_externally_visible_p will always return true then. I rely on the facts that externally_visible flags are initialized to false at the beginning. Is this reasonable? > > We might get around by not streaming externally_visible flag and not > clearling > it when in LTO mode (since it would not be set for other reasons) but > this is > bit fragile. > > We already have flag used_from_other_partition, this situation is > similar, just > it is not used from other partition but externally. Perhaps we can > just add specific > flag for this purpose and update cgraph_externally_visible_p to test > for it? > > Honza Thanks, Bingfeng ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: PING: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-18 16:56 ` Bingfeng Mei @ 2010-06-18 17:25 ` Jan Hubicka 2010-06-21 13:15 ` Bingfeng Mei 2010-06-28 10:10 ` Bingfeng Mei 0 siblings, 2 replies; 27+ messages in thread From: Jan Hubicka @ 2010-06-18 17:25 UTC (permalink / raw) To: Bingfeng Mei; +Cc: Jan Hubicka, gcc-patches, Richard Guenther > + /* Set externally_visible flags 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; > + } > + } This is executed only with LTO and -fwhole-program, so you would make non-LTO -fwhole-program noop. > > The difference from original code is that when no -fwhole-program specified, the true > value of externally_visible (from pass_ipa_function_and_variable_visibility) > will be passed on and not conditionally set to false in pass_ipa_whole_program_visibility. > But does that matter? Cgraph_externally_visible_p will always return true then. Without -fwhole-program the visibilities should match, so this is not a problem, but we need to keep non-LTO -fwhole-program working. It seems that having specific flag for this case is better than trying to play rather fragile games with setting/preserving the flag based on compliation mode. It is useful to know if the symbol is bound from non-LTO object at linktime also to make hidden symbols to become local when building the final DSO even without -fwhole-program. Honza ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: PING: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-18 17:25 ` Jan Hubicka @ 2010-06-21 13:15 ` Bingfeng Mei 2010-06-28 10:10 ` Bingfeng Mei 1 sibling, 0 replies; 27+ messages in thread From: Bingfeng Mei @ 2010-06-21 13:15 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Richard Guenther [-- Attachment #1: Type: text/plain, Size: 3521 bytes --] Honza, As you suggested, I added "externally_visible_by_resolver" flags only for this purpose. The relevant places in ipa.c are updated to check this flag. It passes tests and is bootstrapped. OK for trunk? Thanks, Bingfeng 2010-06-21 Bingfeng Mei <bmei@broadcom.com> * lto-symbtab.c (lto_symtab_merge_decls_1): Set externally_visible_by_resolver 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 flag of varpool_node if externally_visible_by_resolver flag is set. (cgraph_externally_visible_p): check externally_visible_by_resolver flag. * cgraph.h (struct varpool_node): new externally_visible_by_resolver flag. (struct cgraph_local_info): new externally_visible_by_resolver flag. * cgraph.c (dump_cgraph_node): dump externally_visible_by_resolver flag. (cgraph_clone_node): initialize externally_visible_by_resolver. (cgraph_create_virtual_clone): initialize externally_visible_by_resolver. * doc/invoke.texi (-fwhole-program option): Change description of externally_visible attribute accordingly. * doc/extend.texi (externally_visible): Ditto. > -----Original Message----- > From: Jan Hubicka [mailto:hubicka@ucw.cz] > Sent: 18 June 2010 17:39 > To: Bingfeng Mei > Cc: Jan Hubicka; gcc-patches@gcc.gnu.org; Richard Guenther > Subject: Re: PING: [PATCH, LTO] add externally_visible attribute when > necessary with -fwhole-program and resolution file. > > > + /* Set externally_visible flags 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; > > + } > > + } > > This is executed only with LTO and -fwhole-program, so you would make > non-LTO > -fwhole-program noop. > > > > The difference from original code is that when no -fwhole-program > specified, the true > > value of externally_visible (from > pass_ipa_function_and_variable_visibility) > > will be passed on and not conditionally set to false in > pass_ipa_whole_program_visibility. > > But does that matter? Cgraph_externally_visible_p will always return > true then. > > Without -fwhole-program the visibilities should match, so this is not a > problem, but we need to keep non-LTO -fwhole-program working. It seems > that > having specific flag for this case is better than trying to play rather > fragile > games with setting/preserving the flag based on compliation mode. > > It is useful to know if the symbol is bound from non-LTO object at > linktime > also to make hidden symbols to become local when building the final DSO > even > without -fwhole-program. > > Honza [-- Attachment #2: patch --] [-- Type: application/octet-stream, Size: 6937 bytes --] Index: doc/extend.texi =================================================================== --- doc/extend.texi (revision 161065) +++ doc/extend.texi (working copy) @@ -2296,7 +2296,7 @@ @cindex @code{externally_visible} attribute. This attribute, attached to a global variable or function, nullifies the effect of the @option{-fwhole-program} command-line option, so the -object remains visible outside the current compilation unit. +object remains visible outside the current compilation unit. If @option{-fwhole-program} is used together with @option{-flto} and @command{gold} is used as the linker plugin, @code{externally_visible} attributes are automatically added to functions (not variable yet due to a current @command{gold} issue) that are accessed outside of LTO objects according to resolution file produced by @command{gold}. For other linkers that cannot generate resolution file, explicit @code{externally_visible} attributes are still necessary. @item far @cindex functions which handle memory bank switching Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 161065) +++ doc/invoke.texi (working copy) @@ -7294,7 +7294,7 @@ Assume that the current compilation unit represents the whole program being compiled. All public functions and variables with the exception of @code{main} and those merged by attribute @code{externally_visible} become static functions -and in effect are optimized more aggressively by interprocedural optimizers. +and in effect are optimized more aggressively by interprocedural optimizers. If @command{gold} is used as the linker plugin, @code{externally_visible} attributes are automatically added to functions (not variable yet due to a current @command{gold} issue) that are accessed outside of LTO objects according to resolution file produced by @command{gold}. For other linkers that cannot generate resolution file, explicit @code{externally_visible} attributes are still necessary. While this option is equivalent to proper use of the @code{static} keyword for programs consisting of a single file, in combination with option @option{-combine}, @option{-flto} or @option{-fwhopr} this flag can be used to Index: lto-symtab.c =================================================================== --- lto-symtab.c (revision 161065) +++ lto-symtab.c (working copy) @@ -530,11 +530,20 @@ 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 +707,24 @@ && TREE_CODE (prevailing->decl) != VAR_DECL) prevailing->next = NULL; + /* Set externally_visible flags 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_by_resolver = true; + else + prevailing->vnode->externally_visible_by_resolver = true; + } + else if (prevailing->resolution == LDPR_PREVAILING_DEF_IRONLY) + { + if (TREE_CODE (prevailing->decl) == FUNCTION_DECL) + prevailing->node->local.externally_visible_by_resolver = false; + else + prevailing->vnode->externally_visible_by_resolver = false; + } + } return 1; } Index: cgraph.c =================================================================== --- cgraph.c (revision 161065) +++ cgraph.c (working copy) @@ -1825,6 +1825,8 @@ fprintf (f, " local"); if (node->local.externally_visible) fprintf (f, " externally_visible"); + if (node->local.externally_visible_by_resolver) + fprintf (f, " externally_visible_by_resolver"); if (node->local.finalized) fprintf (f, " finalized"); if (node->local.disregard_inline_limits) @@ -2075,6 +2077,7 @@ new_node->analyzed = n->analyzed; new_node->local = n->local; new_node->local.externally_visible = false; + new_node->local.externally_visible_by_resolver = false; new_node->local.local = true; new_node->local.vtable_method = false; new_node->global = n->global; @@ -2266,6 +2269,7 @@ else new_node->clone.combined_args_to_skip = args_to_skip; new_node->local.externally_visible = 0; + new_node->local.externally_visible_by_resolver = 0; new_node->local.local = 1; new_node->lowered = true; new_node->reachable = true; Index: cgraph.h =================================================================== --- cgraph.h (revision 161065) +++ cgraph.h (working copy) @@ -102,6 +102,9 @@ /* Set when function is visible by other units. */ unsigned externally_visible : 1; + /* Set when resolver determines that function is visible by other units. */ + unsigned externally_visible_by_resolver : 1; + /* Set once it has been finalized so we consider it to be output. */ unsigned finalized : 1; @@ -487,6 +490,8 @@ unsigned output : 1; /* Set when function is visible by other units. */ unsigned externally_visible : 1; + /* Set when resolver determines that variable is visible by other units. */ + unsigned externally_visible_by_resolver : 1; /* Set for aliases once they got through assemble_alias. Also set for extra name aliases in varpool_extra_name_alias. */ unsigned alias : 1; Index: ipa.c =================================================================== --- ipa.c (revision 161065) +++ ipa.c (working copy) @@ -576,6 +576,8 @@ return false; if (!whole_program) return true; + if (node->local.externally_visible_by_resolver) + return true; if (DECL_PRESERVE_P (node->decl)) return true; /* COMDAT functions must be shared only if they have address taken, @@ -729,6 +731,7 @@ we start reordering datastructures. */ || DECL_COMDAT (vnode->decl) || DECL_WEAK (vnode->decl) + || vnode->externally_visible_by_resolver || lookup_attribute ("externally_visible", DECL_ATTRIBUTES (vnode->decl)))) vnode->externally_visible = true; ^ permalink raw reply [flat|nested] 27+ messages in thread
* PING: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-18 17:25 ` Jan Hubicka 2010-06-21 13:15 ` Bingfeng Mei @ 2010-06-28 10:10 ` Bingfeng Mei 2010-06-28 10:25 ` Jan Hubicka 1 sibling, 1 reply; 27+ messages in thread From: Bingfeng Mei @ 2010-06-28 10:10 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Richard Guenther [-- Attachment #1: Type: text/plain, Size: 1453 bytes --] Honza, Could you comment on the modified patch? As you suggested, I added new "externally_visible_by_resolver" flags only for this purpose. The relevant places in ipa.c are updated to check this flag. It passes tests and is bootstrapped. OK for trunk? Thanks, Bingfeng 2010-06-21 Bingfeng Mei <bmei@broadcom.com> * lto-symbtab.c (lto_symtab_merge_decls_1): Set externally_visible_by_resolver 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 flag of varpool_node if externally_visible_by_resolver flag is set. (cgraph_externally_visible_p): check externally_visible_by_resolver flag. * cgraph.h (struct varpool_node): new externally_visible_by_resolver flag. (struct cgraph_local_info): new externally_visible_by_resolver flag. * cgraph.c (dump_cgraph_node): dump externally_visible_by_resolver flag. (cgraph_clone_node): initialize externally_visible_by_resolver. (cgraph_create_virtual_clone): initialize externally_visible_by_resolver. * doc/invoke.texi (-fwhole-program option): Change description of externally_visible attribute accordingly. * doc/extend.texi (externally_visible): Ditto. [-- Attachment #2: patch --] [-- Type: application/octet-stream, Size: 6937 bytes --] Index: doc/extend.texi =================================================================== --- doc/extend.texi (revision 161065) +++ doc/extend.texi (working copy) @@ -2296,7 +2296,7 @@ @cindex @code{externally_visible} attribute. This attribute, attached to a global variable or function, nullifies the effect of the @option{-fwhole-program} command-line option, so the -object remains visible outside the current compilation unit. +object remains visible outside the current compilation unit. If @option{-fwhole-program} is used together with @option{-flto} and @command{gold} is used as the linker plugin, @code{externally_visible} attributes are automatically added to functions (not variable yet due to a current @command{gold} issue) that are accessed outside of LTO objects according to resolution file produced by @command{gold}. For other linkers that cannot generate resolution file, explicit @code{externally_visible} attributes are still necessary. @item far @cindex functions which handle memory bank switching Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 161065) +++ doc/invoke.texi (working copy) @@ -7294,7 +7294,7 @@ Assume that the current compilation unit represents the whole program being compiled. All public functions and variables with the exception of @code{main} and those merged by attribute @code{externally_visible} become static functions -and in effect are optimized more aggressively by interprocedural optimizers. +and in effect are optimized more aggressively by interprocedural optimizers. If @command{gold} is used as the linker plugin, @code{externally_visible} attributes are automatically added to functions (not variable yet due to a current @command{gold} issue) that are accessed outside of LTO objects according to resolution file produced by @command{gold}. For other linkers that cannot generate resolution file, explicit @code{externally_visible} attributes are still necessary. While this option is equivalent to proper use of the @code{static} keyword for programs consisting of a single file, in combination with option @option{-combine}, @option{-flto} or @option{-fwhopr} this flag can be used to Index: lto-symtab.c =================================================================== --- lto-symtab.c (revision 161065) +++ lto-symtab.c (working copy) @@ -530,11 +530,20 @@ 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 +707,24 @@ && TREE_CODE (prevailing->decl) != VAR_DECL) prevailing->next = NULL; + /* Set externally_visible flags 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_by_resolver = true; + else + prevailing->vnode->externally_visible_by_resolver = true; + } + else if (prevailing->resolution == LDPR_PREVAILING_DEF_IRONLY) + { + if (TREE_CODE (prevailing->decl) == FUNCTION_DECL) + prevailing->node->local.externally_visible_by_resolver = false; + else + prevailing->vnode->externally_visible_by_resolver = false; + } + } return 1; } Index: cgraph.c =================================================================== --- cgraph.c (revision 161065) +++ cgraph.c (working copy) @@ -1825,6 +1825,8 @@ fprintf (f, " local"); if (node->local.externally_visible) fprintf (f, " externally_visible"); + if (node->local.externally_visible_by_resolver) + fprintf (f, " externally_visible_by_resolver"); if (node->local.finalized) fprintf (f, " finalized"); if (node->local.disregard_inline_limits) @@ -2075,6 +2077,7 @@ new_node->analyzed = n->analyzed; new_node->local = n->local; new_node->local.externally_visible = false; + new_node->local.externally_visible_by_resolver = false; new_node->local.local = true; new_node->local.vtable_method = false; new_node->global = n->global; @@ -2266,6 +2269,7 @@ else new_node->clone.combined_args_to_skip = args_to_skip; new_node->local.externally_visible = 0; + new_node->local.externally_visible_by_resolver = 0; new_node->local.local = 1; new_node->lowered = true; new_node->reachable = true; Index: cgraph.h =================================================================== --- cgraph.h (revision 161065) +++ cgraph.h (working copy) @@ -102,6 +102,9 @@ /* Set when function is visible by other units. */ unsigned externally_visible : 1; + /* Set when resolver determines that function is visible by other units. */ + unsigned externally_visible_by_resolver : 1; + /* Set once it has been finalized so we consider it to be output. */ unsigned finalized : 1; @@ -487,6 +490,8 @@ unsigned output : 1; /* Set when function is visible by other units. */ unsigned externally_visible : 1; + /* Set when resolver determines that variable is visible by other units. */ + unsigned externally_visible_by_resolver : 1; /* Set for aliases once they got through assemble_alias. Also set for extra name aliases in varpool_extra_name_alias. */ unsigned alias : 1; Index: ipa.c =================================================================== --- ipa.c (revision 161065) +++ ipa.c (working copy) @@ -576,6 +576,8 @@ return false; if (!whole_program) return true; + if (node->local.externally_visible_by_resolver) + return true; if (DECL_PRESERVE_P (node->decl)) return true; /* COMDAT functions must be shared only if they have address taken, @@ -729,6 +731,7 @@ we start reordering datastructures. */ || DECL_COMDAT (vnode->decl) || DECL_WEAK (vnode->decl) + || vnode->externally_visible_by_resolver || lookup_attribute ("externally_visible", DECL_ATTRIBUTES (vnode->decl)))) vnode->externally_visible = true; ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: PING: [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-28 10:10 ` Bingfeng Mei @ 2010-06-28 10:25 ` Jan Hubicka 2010-06-28 12:22 ` Committed : " Bingfeng Mei 0 siblings, 1 reply; 27+ messages in thread From: Jan Hubicka @ 2010-06-28 10:25 UTC (permalink / raw) To: Bingfeng Mei; +Cc: Jan Hubicka, gcc-patches, Richard Guenther > Honza, > > Could you comment on the modified patch? As you suggested, I added > new "externally_visible_by_resolver" flags only for this purpose. > The relevant places in ipa.c are updated to check this flag. It passes tests > and is bootstrapped. OK for trunk? The patch is OK. Since we already have used_from_other_partition for similar purpose, I guess it would be better to call the flag used_from_object_file. Honza > > Thanks, > Bingfeng > > 2010-06-21 Bingfeng Mei <bmei@broadcom.com> > > * lto-symbtab.c (lto_symtab_merge_decls_1): Set > externally_visible_by_resolver 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 > flag of varpool_node if externally_visible_by_resolver flag is set. > (cgraph_externally_visible_p): check externally_visible_by_resolver > flag. > * cgraph.h (struct varpool_node): new externally_visible_by_resolver > flag. (struct cgraph_local_info): new externally_visible_by_resolver > flag. > * cgraph.c (dump_cgraph_node): dump externally_visible_by_resolver > flag. (cgraph_clone_node): initialize externally_visible_by_resolver. > (cgraph_create_virtual_clone): initialize externally_visible_by_resolver. > * doc/invoke.texi (-fwhole-program option): Change description of > externally_visible attribute accordingly. > * doc/extend.texi (externally_visible): Ditto. > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Committed : [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file. 2010-06-28 10:25 ` Jan Hubicka @ 2010-06-28 12:22 ` Bingfeng Mei 0 siblings, 0 replies; 27+ messages in thread From: Bingfeng Mei @ 2010-06-28 12:22 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Richard Guenther Changed names of flags and committed at r161483 2010-06-28 Bingfeng Mei <bmei@broadcom.com> * cgraph.h (struct varpool_node): new used_from_object_file flag. (struct cgraph_local_info): new used_from_object_file flag. * cgraph.c (dump_cgraph_node): dump used_from_object_file flag. (cgraph_clone_node): initialize used_from_object_file. (cgraph_create_virtual_clone): initialize used_from_object_file. * lto-symbtab.c (lto_symtab_merge_decls_1): Set used_from_object_file 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 flag of varpool_node if used_from_object_file flag is set. (cgraph_externally_visible_p): check used_from_object_file flag. * doc/invoke.texi (-fwhole-program option): Change description of externally_visible attribute accordingly. * doc/extend.texi (externally_visible): Ditto. > -----Original Message----- > From: Jan Hubicka [mailto:hubicka@ucw.cz] > Sent: 28 June 2010 10:47 > To: Bingfeng Mei > Cc: Jan Hubicka; gcc-patches@gcc.gnu.org; Richard Guenther > Subject: Re: PING: [PATCH, LTO] add externally_visible attribute when > necessary with -fwhole-program and resolution file. > > > Honza, > > > > Could you comment on the modified patch? As you suggested, I added > > new "externally_visible_by_resolver" flags only for this purpose. > > The relevant places in ipa.c are updated to check this flag. It > passes tests > > and is bootstrapped. OK for trunk? > > The patch is OK. Since we already have used_from_other_partition for > similar purpose, > I guess it would be better to call the flag used_from_object_file. > > Honza > > > > Thanks, > > Bingfeng > > > > 2010-06-21 Bingfeng Mei <bmei@broadcom.com> > > > > * lto-symbtab.c (lto_symtab_merge_decls_1): Set > > externally_visible_by_resolver 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 > > flag of varpool_node if externally_visible_by_resolver flag > is set. > > (cgraph_externally_visible_p): check > externally_visible_by_resolver > > flag. > > * cgraph.h (struct varpool_node): new > externally_visible_by_resolver > > flag. (struct cgraph_local_info): new > externally_visible_by_resolver > > flag. > > * cgraph.c (dump_cgraph_node): dump > externally_visible_by_resolver > > flag. (cgraph_clone_node): initialize > externally_visible_by_resolver. > > (cgraph_create_virtual_clone): initialize > externally_visible_by_resolver. > > * doc/invoke.texi (-fwhole-program option): Change > description of > > externally_visible attribute accordingly. > > * doc/extend.texi (externally_visible): Ditto. > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2010-06-28 10:44 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-06-09 15:02 [PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file Bingfeng Mei 2010-06-09 15:13 ` Richard Guenther 2010-06-09 15:25 ` Bingfeng Mei 2010-06-09 15:33 ` Richard Guenther 2010-06-09 15:35 ` Bingfeng Mei 2010-06-09 15:32 ` Bingfeng Mei 2010-06-09 15:48 ` Richard Guenther 2010-06-10 10:30 ` Bingfeng Mei 2010-06-10 10:35 ` Richard Guenther 2010-06-10 15:31 ` Bingfeng Mei 2010-06-10 17:07 ` Richard Guenther 2010-06-10 17:10 ` Bingfeng Mei 2010-06-11 9:34 ` Richard Guenther 2010-06-14 18:58 ` Cary Coutant 2010-06-14 20:03 ` Richard Guenther 2010-06-14 9:17 ` Bingfeng Mei 2010-06-14 10:10 ` Richard Guenther 2010-06-14 11:33 ` Bingfeng Mei 2010-06-14 15:36 ` Bingfeng Mei 2010-06-18 9:28 ` PING: " Bingfeng Mei 2010-06-18 16:46 ` Jan Hubicka 2010-06-18 16:56 ` Bingfeng Mei 2010-06-18 17:25 ` Jan Hubicka 2010-06-21 13:15 ` Bingfeng Mei 2010-06-28 10:10 ` Bingfeng Mei 2010-06-28 10:25 ` Jan Hubicka 2010-06-28 12:22 ` Committed : " Bingfeng Mei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).