Patchwork [LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file.

login
register
mail settings
Submitter Bingfeng Mei
Date June 14, 2010, 8:39 a.m.
Message ID <7FB04A5C213E9943A72EE127DB74F0ADA6667D06B5@SJEXCHCCR02.corp.ad.broadcom.com>
Download mbox | patch
Permalink /patch/55492/
State New
Headers show

Comments

Bingfeng Mei - June 14, 2010, 8:39 a.m.
Hi, Richard,
Here is the updated patch. The flags are set instead of attributes now. 
The check is moved to the end of lto_symtab_merge_decls_1. For the DECL_COMM,
since internal resolver is always used due to your workaround for gold plugin,
These variables would still need explicit externally_visible attributes. 

Bootstrapped and tested.

Cheers,
Bingfeng.

2010-06-11  Bingfeng Mei <bmei@broadcom.com>

	* lto-symbtab.c (lto_symtab_merge_decls_1): Set externally_visible
        flags for symbols of LDPR_PREVAILING_DEF when compiling with
	-fwhole-program 
	(lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY for 
	internal resolver
        * ipa.c (function_and_variable_visibility): Set externally_visible
        flags only if they are false. This allows flags to be passed from



> -----Original Message-----
> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> Sent: 09 June 2010 16:26
> To: Bingfeng Mei
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> necessary with -fwhole-program and resolution file.
> 
> On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
> > I added an attribute because -fwhole-program/externally_visible is
> handled in ipa.c
> >
> > ...
> >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES (node-
> >decl)))
> >    return true;
> > ...
> >
> > Adding attribute seems cleaner than changing flags, otherwise I need
> to change
> > handling in ipa.c as well.
> 
> True, but there is an externally_visible flag in cgraph_node,
> so I guess that attribute lookup is bogus.
> 
> Richard.
> 
> > Bingfeng
> >
> >> -----Original Message-----
> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> Sent: 09 June 2010 16:02
> >> To: Bingfeng Mei
> >> Cc: gcc-patches@gcc.gnu.org
> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> >> necessary with -fwhole-program and resolution file.
> >>
> >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei <bmei@broadcom.com>
> wrote:
> >> > Hi,
> >> > This patch addresses issue discussed in
> >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html
> >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html
> >> >
> >> > With the patch, any declaration which is resolved as
> >> LDPR_PREVAILING_DEF
> >> > and compiled with -fwhole-program is annotated with
> >> > attribute "externally_visible" if it doesn't exist already.
> >> > This eliminates the error-prone process of manual annotation
> >> > of the attribute when compiling mixed LTO/non-LTO applications.
> >> >
> >> > For the following test files:
> >> > a.c
> >> >
> >> > #include <string.h>
> >> > #include <stdio.h>
> >> > extern int foo(int);
> >> > /* Called by b.c, should not be optimized by -fwhole-program */
> >> > void bar()
> >> > {
> >> >  printf("bar\n");
> >> > }
> >> > /* Not used by others, should be optimized out by -fwhole-
> program*/
> >> > void bar2()
> >> > {
> >> >  printf("bar2\n");
> >> > }
> >> > extern int src[], dst[];
> >> > int vvvvvv;
> >> > int main()
> >> > {
> >> >  int ret;
> >> >  vvvvvv = 12;
> >> >  ret = foo(20);
> >> >  bar2();
> >> >  memcpy(dst, src, 100);
> >> >  return ret + 3;
> >> > }
> >> >
> >> > b.c
> >> >
> >> > #include <stdio.h>
> >> > int src[100];
> >> > int dst[100];
> >> > extern int vvvvvv;
> >> > extern void bar();
> >> > int foo(int c)
> >> > {
> >> >  printf("Hello world: %d\n", c);
> >> >  bar();
> >> >  return 1000 + vvvvvv;
> >> > }
> >> >
> >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto
> >> > ~/work/install-x86/bin/gcc  b.c -O2 -c
> >> > ar cru libb.a b.o
> >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-linker-
> plugin
> >> -o f -fwhole-program
> >> >
> >> > The code is compiled and linked correctly. bar & vvvvvv don't
> become
> >> static function
> >> > and cause link errors, whereas bar2 is inlined as expected.
> >> >
> >> >
> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >> >
> >> > Cheers,
> >> > Bingfeng Mei
> >> >
> >> > 2010-06-09  Bingfeng Mei <bmei@broadcom.com>
> >> >
> >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add
> >> externally_visible
> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
> >> compiling with
> >> >        -fwhole-program
> >> >
> >> >
> >> > Index: lto-symtab.c
> >> >
> ===================================================================
> >> > --- lto-symtab.c        (revision 160463)
> >> > +++ lto-symtab.c        (working copy)
> >> > @@ -476,7 +476,19 @@
> >> >
> >> >   /* If the chain is already resolved there is nothing else to
> >> do.  */
> >> >   if (e->resolution != LDPR_UNKNOWN)
> >> > -    return;
> >> > +    {
> >> > +      /* Add externally_visible attribute for declaration of
> >> LDPR_PREVAILING_DEF */
> >> > +      if (e->resolution == LDPR_PREVAILING_DEF &&
> flag_whole_program)
> >> > +        {
> >> > +          if (!lookup_attribute ("externally_visible",
> >> DECL_ATTRIBUTES (e->decl)))
> >> > +            {
> >> > +              DECL_ATTRIBUTES (e->decl)
> >> > +                = tree_cons (get_identifier
> ("externally_visible"),
> >> NULL_TREE,
> >> > +                             DECL_ATTRIBUTES (e->decl));
> >>
> >> I don't think we need to add an attribute here but we can play
> >> with some cgraph flags which is cheaper.
> >>
> >> Also I think this isn't really correct - not everything that
> prevails
> >> needs to be externally visible (in fact, you seem to simply
> >> remove the effect of -fwhole-program completely).
> >>
> >> A testcase that should still work is
> >>
> >> t1.c:
> >> void foo(void) { bar (); }
> >> t2.c
> >> extern void foo (void);
> >> void bar (void) {}
> >> void eliminate_me (void) {}
> >> int main()
> >> {
> >>    foo();
> >> }
> >>
> >> and eliminate_me should still be eliminated with -fwhole-program
> >> if you do
> >>
> >> gcc -c t1.c
> >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin
> >>
> >> Thus, the resolution file probably does not have the information
> >> you need (a list of references from outside of the LTO file set).
> >>
> >> Richard.
> >
> >
> >
Richard Guenther - June 14, 2010, 9:24 a.m.
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.
>> >
>> >
>> >
>
>
>
Bingfeng Mei - June 14, 2010, 10:35 a.m.
I will amend the LTO documents. 

With/without my patch, GCC produces the same results compiling
for IR only for my examples. And it passes LTO tests.  However, bar2 
and foo functions are not inlined as I expected using GNU ld. I believe
this is due to the "externally_visible" flags are improperly passed
from compile-time to link time as discussed in previous mails in this
thread, and this won't be affected by my patch. I will send a separate patch.


Cheers,
Bingfeng




> -----Original Message-----
> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> Sent: 14 June 2010 10:25
> To: Bingfeng Mei
> Cc: gcc-patches@gcc.gnu.org; Jan Hubicka
> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> necessary with -fwhole-program and resolution file.
> 
> On Mon, Jun 14, 2010 at 10:39 AM, Bingfeng Mei <bmei@broadcom.com>
> wrote:
> > Hi, Richard,
> > Here is the updated patch. The flags are set instead of attributes
> now.
> > The check is moved to the end of lto_symtab_merge_decls_1. For the
> DECL_COMM,
> > since internal resolver is always used due to your workaround for
> gold plugin,
> > These variables would still need explicit externally_visible
> attributes.
> 
> Can you amend the docs that talk about -flto + -fwhole-program
> accordingly?
> 
> I'd like Honza to go over the cgraph related changes, otherwise
> the patch looks good with ...
> 
> > Bootstrapped and tested.
> >
> > Cheers,
> > Bingfeng.
> >
> > 2010-06-11  Bingfeng Mei <bmei@broadcom.com>
> >
> >        * lto-symbtab.c (lto_symtab_merge_decls_1): Set
> externally_visible
> >        flags for symbols of LDPR_PREVAILING_DEF when compiling with
> >        -fwhole-program
> >        (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY
> for
> >        internal resolver
> >        * ipa.c (function_and_variable_visibility): Set
> externally_visible
> >        flags only if they are false. This allows flags to be passed
> from
> >
> >
> > Index: lto-symtab.c
> > ===================================================================
> > --- lto-symtab.c        (revision 160529)
> > +++ lto-symtab.c        (working copy)
> > @@ -530,11 +530,21 @@
> >     return;
> >
> >  found:
> > -  if (TREE_CODE (prevailing->decl) == VAR_DECL
> > -      && TREE_READONLY (prevailing->decl))
> > +  /* If current lto files represent the whole program,
> > +    it is correct to use LDPR_PREVALING_DEF_IRONLY.
> > +    If current lto files are part of whole program, internal
> > +    resolver doesn't know if it is LDPR_PREVAILING_DEF
> > +    or LDPR_PREVAILING_DEF_IRONLY. Use IRONLY conforms to
> > +    using -fwhole-program. Otherwise, it doesn't
> > +    matter using either LDPR_PREVAILING_DEF or
> > +    LDPR_PREVAILING_DEF_IRONLY
> > +
> > +    FIXME: above workaround due to gold plugin makes some
> > +    variables IRONLY, which are indeed PREVAILING_DEF in
> > +    resolution file. These variables still need manual
> > +    externally_visible attribute
> > +    */
> 
> Full-stop at the end of the sentence, */ not on the next line.  Also
> two spaces after each '.'
> 
> >     prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
> > -  else
> > -    prevailing->resolution = LDPR_PREVAILING_DEF;
> >  }
> >
> >  /* Merge all decls in the symbol table chain to the prevailing decl
> and
> > @@ -698,6 +708,25 @@
> >       && TREE_CODE (prevailing->decl) != VAR_DECL)
> >     prevailing->next = NULL;
> >
> > +
> 
> No extra vertical space please.
> 
> > +  /* Add externally_visible attribute for declaration of
> LDPR_PREVAILING_DEF */
> 
> Adjust the comment, to sth like "In whole-program mode mark
> LDPR_PREVAILING_DEFs as externally visible. "
> 
> > +  if (flag_whole_program)
> > +    {
> > +      if (prevailing->resolution == LDPR_PREVAILING_DEF)
> > +        {
> > +          if (TREE_CODE (prevailing->decl) == FUNCTION_DECL)
> > +            prevailing->node->local.externally_visible = true;
> > +          else
> > +            prevailing->vnode->externally_visible = true;
> > +        }
> > +      else if (prevailing->resolution == LDPR_PREVAILING_DEF_IRONLY)
> > +        {
> > +          if (TREE_CODE (prevailing->decl) == FUNCTION_DECL)
> > +            prevailing->node->local.externally_visible = false;
> > +          else
> > +            prevailing->vnode->externally_visible = false;
> > +        }
> > +    }
> 
> Honza will tell you if the above is correct, I am not 100% sure.
> 
> Did you verify we generate the same code with and without your
> patch when all symbols are resolved inside the IL?
> 
> Thanks,
> Richard.
> 
> >   return 1;
> >  }
> >
> > Index: ipa.c
> > ===================================================================
> > --- ipa.c       (revision 160529)
> > +++ ipa.c       (working copy)
> > @@ -665,13 +665,12 @@
> >        }
> >       gcc_assert ((!DECL_WEAK (node->decl) && !DECL_COMDAT (node-
> >decl))
> >                  || TREE_PUBLIC (node->decl) || DECL_EXTERNAL (node-
> >decl));
> > -      if (cgraph_externally_visible_p (node, whole_program))
> > +      if (!node->local.externally_visible
> > +          && cgraph_externally_visible_p (node, whole_program))
> >         {
> >          gcc_assert (!node->global.inlined_to);
> >          node->local.externally_visible = true;
> >        }
> > -      else
> > -       node->local.externally_visible = false;
> >       if (!node->local.externally_visible && node->analyzed
> >          && !DECL_EXTERNAL (node->decl))
> >        {
> > @@ -721,7 +720,8 @@
> >     {
> >       if (!vnode->finalized)
> >         continue;
> > -      if (vnode->needed
> > +      if (!vnode->externally_visible
> > +          && vnode->needed
> >          && (DECL_COMDAT (vnode->decl) || TREE_PUBLIC (vnode->decl))
> >          && (!whole_program
> >              /* We can privatize comdat readonly variables whose
> address is not taken,
> > @@ -732,8 +732,6 @@
> >              || lookup_attribute ("externally_visible",
> >                                   DECL_ATTRIBUTES (vnode->decl))))
> >        vnode->externally_visible = true;
> > -      else
> > -        vnode->externally_visible = false;
> >       if (!vnode->externally_visible)
> >        {
> >          gcc_assert (whole_program || !TREE_PUBLIC (vnode->decl));
> >
> >> -----Original Message-----
> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> Sent: 09 June 2010 16:26
> >> To: Bingfeng Mei
> >> Cc: gcc-patches@gcc.gnu.org
> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> >> necessary with -fwhole-program and resolution file.
> >>
> >> On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei <bmei@broadcom.com>
> wrote:
> >> > I added an attribute because -fwhole-program/externally_visible is
> >> handled in ipa.c
> >> >
> >> > ...
> >> >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES
> (node-
> >> >decl)))
> >> >    return true;
> >> > ...
> >> >
> >> > Adding attribute seems cleaner than changing flags, otherwise I
> need
> >> to change
> >> > handling in ipa.c as well.
> >>
> >> True, but there is an externally_visible flag in cgraph_node,
> >> so I guess that attribute lookup is bogus.
> >>
> >> Richard.
> >>
> >> > Bingfeng
> >> >
> >> >> -----Original Message-----
> >> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> >> Sent: 09 June 2010 16:02
> >> >> To: Bingfeng Mei
> >> >> Cc: gcc-patches@gcc.gnu.org
> >> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> >> >> necessary with -fwhole-program and resolution file.
> >> >>
> >> >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei <bmei@broadcom.com>
> >> wrote:
> >> >> > Hi,
> >> >> > This patch addresses issue discussed in
> >> >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html
> >> >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html
> >> >> >
> >> >> > With the patch, any declaration which is resolved as
> >> >> LDPR_PREVAILING_DEF
> >> >> > and compiled with -fwhole-program is annotated with
> >> >> > attribute "externally_visible" if it doesn't exist already.
> >> >> > This eliminates the error-prone process of manual annotation
> >> >> > of the attribute when compiling mixed LTO/non-LTO applications.
> >> >> >
> >> >> > For the following test files:
> >> >> > a.c
> >> >> >
> >> >> > #include <string.h>
> >> >> > #include <stdio.h>
> >> >> > extern int foo(int);
> >> >> > /* Called by b.c, should not be optimized by -fwhole-program */
> >> >> > void bar()
> >> >> > {
> >> >> >  printf("bar\n");
> >> >> > }
> >> >> > /* Not used by others, should be optimized out by -fwhole-
> >> program*/
> >> >> > void bar2()
> >> >> > {
> >> >> >  printf("bar2\n");
> >> >> > }
> >> >> > extern int src[], dst[];
> >> >> > int vvvvvv;
> >> >> > int main()
> >> >> > {
> >> >> >  int ret;
> >> >> >  vvvvvv = 12;
> >> >> >  ret = foo(20);
> >> >> >  bar2();
> >> >> >  memcpy(dst, src, 100);
> >> >> >  return ret + 3;
> >> >> > }
> >> >> >
> >> >> > b.c
> >> >> >
> >> >> > #include <stdio.h>
> >> >> > int src[100];
> >> >> > int dst[100];
> >> >> > extern int vvvvvv;
> >> >> > extern void bar();
> >> >> > int foo(int c)
> >> >> > {
> >> >> >  printf("Hello world: %d\n", c);
> >> >> >  bar();
> >> >> >  return 1000 + vvvvvv;
> >> >> > }
> >> >> >
> >> >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto
> >> >> > ~/work/install-x86/bin/gcc  b.c -O2 -c
> >> >> > ar cru libb.a b.o
> >> >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-linker-
> >> plugin
> >> >> -o f -fwhole-program
> >> >> >
> >> >> > The code is compiled and linked correctly. bar & vvvvvv don't
> >> become
> >> >> static function
> >> >> > and cause link errors, whereas bar2 is inlined as expected.
> >> >> >
> >> >> >
> >> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >> >> >
> >> >> > Cheers,
> >> >> > Bingfeng Mei
> >> >> >
> >> >> > 2010-06-09  Bingfeng Mei <bmei@broadcom.com>
> >> >> >
> >> >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add
> >> >> externally_visible
> >> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
> >> >> compiling with
> >> >> >        -fwhole-program
> >> >> >
> >> >> >
> >> >> > Index: lto-symtab.c
> >> >> >
> >> ===================================================================
> >> >> > --- lto-symtab.c        (revision 160463)
> >> >> > +++ lto-symtab.c        (working copy)
> >> >> > @@ -476,7 +476,19 @@
> >> >> >
> >> >> >   /* If the chain is already resolved there is nothing else to
> >> >> do.  */
> >> >> >   if (e->resolution != LDPR_UNKNOWN)
> >> >> > -    return;
> >> >> > +    {
> >> >> > +      /* Add externally_visible attribute for declaration of
> >> >> LDPR_PREVAILING_DEF */
> >> >> > +      if (e->resolution == LDPR_PREVAILING_DEF &&
> >> flag_whole_program)
> >> >> > +        {
> >> >> > +          if (!lookup_attribute ("externally_visible",
> >> >> DECL_ATTRIBUTES (e->decl)))
> >> >> > +            {
> >> >> > +              DECL_ATTRIBUTES (e->decl)
> >> >> > +                = tree_cons (get_identifier
> >> ("externally_visible"),
> >> >> NULL_TREE,
> >> >> > +                             DECL_ATTRIBUTES (e->decl));
> >> >>
> >> >> I don't think we need to add an attribute here but we can play
> >> >> with some cgraph flags which is cheaper.
> >> >>
> >> >> Also I think this isn't really correct - not everything that
> >> prevails
> >> >> needs to be externally visible (in fact, you seem to simply
> >> >> remove the effect of -fwhole-program completely).
> >> >>
> >> >> A testcase that should still work is
> >> >>
> >> >> t1.c:
> >> >> void foo(void) { bar (); }
> >> >> t2.c
> >> >> extern void foo (void);
> >> >> void bar (void) {}
> >> >> void eliminate_me (void) {}
> >> >> int main()
> >> >> {
> >> >>    foo();
> >> >> }
> >> >>
> >> >> and eliminate_me should still be eliminated with -fwhole-program
> >> >> if you do
> >> >>
> >> >> gcc -c t1.c
> >> >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin
> >> >>
> >> >> Thus, the resolution file probably does not have the information
> >> >> you need (a list of references from outside of the LTO file set).
> >> >>
> >> >> Richard.
> >> >
> >> >
> >> >
> >
> >
> >
#include <stdio.h>
int src[100];
int dst[100];
extern int vvvvvv;
extern void bar();
int foo(int c)
{
  printf("Hello world: %d\n", c);
  bar();
  return 1000 + vvvvvv;
}
#include <string.h>
#include <stdio.h>
extern int foo(int);
/* Called by b.c, should not be optimized by -fwhole-program */
void bar() 
{
  printf("bar\n");
}  
/* Not used by others, should be optimized out by -fwhole-program*/
void bar2()
{
  printf("bar2\n");
}
extern int src[], dst[];
int vvvvvv;
int main()
{ 
  int ret;
  vvvvvv = 12; 
  ret = foo(20);
  bar2();
  memcpy(dst, src, 100);
  return ret + 3;
}
Bingfeng Mei - June 14, 2010, 3:06 p.m.
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.
> >> >
> >> >
> >> >
> >
> >
> >
Bingfeng Mei - June 18, 2010, 8:58 a.m.
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.
> >> >
> >> >
> >> >
> >
> >
> >

Patch

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));