diff mbox

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

Message ID 20100618144840.GA18733@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka June 18, 2010, 2:48 p.m. UTC 
pass_ipa_whole_program_visibility.

This is so to allow output from early IPA passes and early local optimization
to be used by both at the compile time and link time.  So variables/functions
are externally visible per their visiblities first and then in
pass_ipa_whole_program_visibility they might become static.

This is why the conditional sets externally_visible to false.  Not doing so you
effectively disbale -fwhole-program.

We might get around by not streaming externally_visible flag and not clearling
it when in LTO mode (since it would not be set for other reasons) but this is
bit fragile.

We already have flag used_from_other_partition, this situation is similar, just
it is not used from other partition but externally.  Perhaps we can just add specific
flag for this purpose and update cgraph_externally_visible_p to test for it?

Honza

Comments

Bingfeng Mei June 18, 2010, 4:24 p.m. UTC | #1 
> -----Original Message-----
> From: Jan Hubicka [mailto:hubicka@ucw.cz]
> Sent: 18 June 2010 15:49
> To: Bingfeng Mei
> Cc: Jan Hubicka; gcc-patches@gcc.gnu.org; Richard Guenther
> Subject: Re: PING: [PATCH, LTO] add externally_visible attribute when
> necessary with -fwhole-program and resolution file.
> 
> 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;
> 
> This is not correct: the externally visible attribute is computed twice;
> once
> in pass_ipa_function_and_variable_visibility and second time in
> pass_ipa_whole_program_visibility.
> 
> This is so to allow output from early IPA passes and early local
> optimization
> to be used by both at the compile time and link time.  So
> variables/functions
> are externally visible per their visiblities first and then in
> pass_ipa_whole_program_visibility they might become static.
> 
> This is why the conditional sets externally_visible to false.  Not
> doing so you
> effectively disbale -fwhole-program.

Actually, the externally_visible flags will always be set to either
true or false with -fwhole-program due to following code. The unpacked
values from LTO bytecode won't be passed though to effectively 
disable -fwhole-program.

+  /* Set externally_visible flags 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;
+        }
+    }

The difference from original code is that when no -fwhole-program specified, the true
value of externally_visible (from pass_ipa_function_and_variable_visibility)
will be passed on and not conditionally set to false in pass_ipa_whole_program_visibility.
But does that matter? Cgraph_externally_visible_p will always return true then.

I rely on the facts that externally_visible flags are initialized to false
at the beginning. Is this reasonable?

> 
> We might get around by not streaming externally_visible flag and not
> clearling
> it when in LTO mode (since it would not be set for other reasons) but
> this is
> bit fragile.
> 
> We already have flag used_from_other_partition, this situation is
> similar, just
> it is not used from other partition but externally.  Perhaps we can
> just add specific
> flag for this purpose and update cgraph_externally_visible_p to test
> for it?
> 

> Honza


Thanks,
Bingfeng
Jan Hubicka June 18, 2010, 4:39 p.m. UTC | #2 
> +  /* Set externally_visible flags 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;
> +        }
> +    }

This is executed only with LTO and -fwhole-program, so you would make non-LTO
-fwhole-program noop.
> 
> The difference from original code is that when no -fwhole-program specified, the true
> value of externally_visible (from pass_ipa_function_and_variable_visibility)
> will be passed on and not conditionally set to false in pass_ipa_whole_program_visibility.
> But does that matter? Cgraph_externally_visible_p will always return true then.

Without -fwhole-program the visibilities should match, so this is not a
problem, but we need to keep non-LTO -fwhole-program working.  It seems that
having specific flag for this case is better than trying to play rather fragile
games with setting/preserving the flag based on compliation mode.

It is useful to know if the symbol is bound from non-LTO object at linktime
also to make hidden symbols to become local when building the final DSO even
without -fwhole-program.

Honza
Bingfeng Mei June 21, 2010, 12:12 p.m. UTC | #3 
Honza, 

As you suggested, I added "externally_visible_by_resolver" flags only for this purpose. 
The relevant places in ipa.c are updated to check this flag. It passes tests
and is bootstrapped. OK for trunk?

Thanks,
Bingfeng

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

        * lto-symbtab.c (lto_symtab_merge_decls_1): Set
        externally_visible_by_resolver 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
        flag of varpool_node if externally_visible_by_resolver flag is set.
        (cgraph_externally_visible_p): check externally_visible_by_resolver
        flag.
        * cgraph.h (struct varpool_node): new externally_visible_by_resolver
        flag.  (struct cgraph_local_info): new externally_visible_by_resolver
        flag.
        * cgraph.c (dump_cgraph_node): dump externally_visible_by_resolver
        flag.  (cgraph_clone_node): initialize externally_visible_by_resolver.
        (cgraph_create_virtual_clone): initialize externally_visible_by_resolver.
        * doc/invoke.texi (-fwhole-program option): Change description of
        externally_visible attribute accordingly.
        * doc/extend.texi (externally_visible): Ditto.

> -----Original Message-----
> From: Jan Hubicka [mailto:hubicka@ucw.cz]
> Sent: 18 June 2010 17:39
> To: Bingfeng Mei
> Cc: Jan Hubicka; gcc-patches@gcc.gnu.org; Richard Guenther
> Subject: Re: PING: [PATCH, LTO] add externally_visible attribute when
> necessary with -fwhole-program and resolution file.
> 
> > +  /* Set externally_visible flags 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;
> > +        }
> > +    }
> 
> This is executed only with LTO and -fwhole-program, so you would make
> non-LTO
> -fwhole-program noop.
> >
> > The difference from original code is that when no -fwhole-program
> specified, the true
> > value of externally_visible (from
> pass_ipa_function_and_variable_visibility)
> > will be passed on and not conditionally set to false in
> pass_ipa_whole_program_visibility.
> > But does that matter? Cgraph_externally_visible_p will always return
> true then.
> 
> Without -fwhole-program the visibilities should match, so this is not a
> problem, but we need to keep non-LTO -fwhole-program working.  It seems
> that
> having specific flag for this case is better than trying to play rather
> fragile
> games with setting/preserving the flag based on compliation mode.
> 
> It is useful to know if the symbol is bound from non-LTO object at
> linktime
> also to make hidden symbols to become local when building the final DSO
> even
> without -fwhole-program.
> 
> Honza
Bingfeng Mei June 28, 2010, 9:08 a.m. UTC | #4 
Honza, 

Could you comment on the modified patch? As you suggested, I added 
new "externally_visible_by_resolver" flags only for this purpose. 
The relevant places in ipa.c are updated to check this flag. It passes tests
and is bootstrapped. OK for trunk?

Thanks,
Bingfeng

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

        * lto-symbtab.c (lto_symtab_merge_decls_1): Set
        externally_visible_by_resolver 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
        flag of varpool_node if externally_visible_by_resolver flag is set.
        (cgraph_externally_visible_p): check externally_visible_by_resolver
        flag.
        * cgraph.h (struct varpool_node): new externally_visible_by_resolver
        flag.  (struct cgraph_local_info): new externally_visible_by_resolver
        flag.
        * cgraph.c (dump_cgraph_node): dump externally_visible_by_resolver
        flag.  (cgraph_clone_node): initialize externally_visible_by_resolver.
        (cgraph_create_virtual_clone): initialize externally_visible_by_resolver.
        * doc/invoke.texi (-fwhole-program option): Change description of
        externally_visible attribute accordingly.
        * doc/extend.texi (externally_visible): Ditto.
Jan Hubicka June 28, 2010, 9:46 a.m. UTC | #5 
> Honza, 
> 
> Could you comment on the modified patch? As you suggested, I added 
> new "externally_visible_by_resolver" flags only for this purpose. 
> The relevant places in ipa.c are updated to check this flag. It passes tests
> and is bootstrapped. OK for trunk?

The patch is OK.  Since we already have used_from_other_partition for similar purpose,
I guess it would be better to call the flag used_from_object_file.

Honza
> 
> Thanks,
> Bingfeng
> 
> 2010-06-21  Bingfeng Mei <bmei@broadcom.com>
> 
>         * lto-symbtab.c (lto_symtab_merge_decls_1): Set
>         externally_visible_by_resolver 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
>         flag of varpool_node if externally_visible_by_resolver flag is set.
>         (cgraph_externally_visible_p): check externally_visible_by_resolver
>         flag.
>         * cgraph.h (struct varpool_node): new externally_visible_by_resolver
>         flag.  (struct cgraph_local_info): new externally_visible_by_resolver
>         flag.
>         * cgraph.c (dump_cgraph_node): dump externally_visible_by_resolver
>         flag.  (cgraph_clone_node): initialize externally_visible_by_resolver.
>         (cgraph_create_virtual_clone): initialize externally_visible_by_resolver.
>         * doc/invoke.texi (-fwhole-program option): Change description of
>         externally_visible attribute accordingly.
>         * doc/extend.texi (externally_visible): Ditto.
> 
>
Bingfeng Mei June 28, 2010, 10:41 a.m. UTC | #6 
Changed names of flags and committed  at r161483

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

        * cgraph.h (struct varpool_node): new used_from_object_file flag.
        (struct cgraph_local_info): new used_from_object_file flag.
        * cgraph.c (dump_cgraph_node): dump used_from_object_file flag.
        (cgraph_clone_node): initialize used_from_object_file.
        (cgraph_create_virtual_clone): initialize used_from_object_file.
        * lto-symbtab.c (lto_symtab_merge_decls_1): Set
        used_from_object_file 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
        flag of varpool_node if used_from_object_file flag is set.
        (cgraph_externally_visible_p): check used_from_object_file flag.
        * doc/invoke.texi (-fwhole-program option): Change description of
        externally_visible attribute accordingly.
        * doc/extend.texi (externally_visible): Ditto.

> -----Original Message-----
> From: Jan Hubicka [mailto:hubicka@ucw.cz]
> Sent: 28 June 2010 10:47
> To: Bingfeng Mei
> Cc: Jan Hubicka; gcc-patches@gcc.gnu.org; Richard Guenther
> Subject: Re: PING: [PATCH, LTO] add externally_visible attribute when
> necessary with -fwhole-program and resolution file.
> 
> > Honza,
> >
> > Could you comment on the modified patch? As you suggested, I added
> > new "externally_visible_by_resolver" flags only for this purpose.
> > The relevant places in ipa.c are updated to check this flag. It
> passes tests
> > and is bootstrapped. OK for trunk?
> 
> The patch is OK.  Since we already have used_from_other_partition for
> similar purpose,
> I guess it would be better to call the flag used_from_object_file.
> 
> Honza
> >
> > Thanks,
> > Bingfeng
> >
> > 2010-06-21  Bingfeng Mei <bmei@broadcom.com>
> >
> >         * lto-symbtab.c (lto_symtab_merge_decls_1): Set
> >         externally_visible_by_resolver 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
> >         flag of varpool_node if externally_visible_by_resolver flag
> is set.
> >         (cgraph_externally_visible_p): check
> externally_visible_by_resolver
> >         flag.
> >         * cgraph.h (struct varpool_node): new
> externally_visible_by_resolver
> >         flag.  (struct cgraph_local_info): new
> externally_visible_by_resolver
> >         flag.
> >         * cgraph.c (dump_cgraph_node): dump
> externally_visible_by_resolver
> >         flag.  (cgraph_clone_node): initialize
> externally_visible_by_resolver.
> >         (cgraph_create_virtual_clone): initialize
> externally_visible_by_resolver.
> >         * doc/invoke.texi (-fwhole-program option): Change
> description of
> >         externally_visible attribute accordingly.
> >         * doc/extend.texi (externally_visible): Ditto.
> >
> >
> 
>
diff mbox

Patch

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;

This is not correct: the externally visible attribute is computed twice; once
in pass_ipa_function_and_variable_visibility and second time in