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

[Richard Guenther]

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.
>> >
>> >
>> >
>
>
>