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

[Richard Guenther <richard dot guenther at gmail dot com>]

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