[PATCH, LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file.
Bingfeng Mei
bmei@broadcom.com
Mon Jun 14 11:33:00 GMT 2010
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.
> >> >
> >> >
> >> >
> >
> >
> >
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: b.c
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20100614/14c8fdf8/attachment.c>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: a.c
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20100614/14c8fdf8/attachment-0001.c>
More information about the Gcc-patches
mailing list