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

["Bingfeng Mei" <bmei at broadcom dot com>]

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