[IPA] Refactor decl localizing

Message ID f32e54bb-e3a5-d3cd-ec03-f315aa15e7a1@acm.org
State New
Headers show

Commit Message

Nathan Sidwell Jan. 6, 2017, 9:25 p.m.
This patch refactors the decl localizing that happens in 
function_and_variable_visibility.  It doesn't fix the bug I'm working on 
(that's next).

Both the FOR_EACH_FUNCTION and FOR_EACH_VARIABLE loops contain very 
similar, but not quite the same code for localizing a definition that 
it's determined need not be externally visible.  It looks to me that the 
not-quite-the-sameness is erroneous, and this patch refactors that code 
into a common subroutine. If the differences need to be maintained 
(slight differences in when unique_name is updated and whether 
resolution is set to LDPR_PREVAILING_DEF_IRONLY), I think a flag to the 
new function would be best, rather than keep the duplicated code.

booted & tested on x86_64-linux, ok?

nathan
-- 
Nathan Sidwell

Comments

Nathan Sidwell Jan. 17, 2017, 12:36 p.m. | #1
Ping?

On 01/06/2017 04:25 PM, Nathan Sidwell wrote:
> This patch refactors the decl localizing that happens in

> function_and_variable_visibility.  It doesn't fix the bug I'm working on

> (that's next).

>

> Both the FOR_EACH_FUNCTION and FOR_EACH_VARIABLE loops contain very

> similar, but not quite the same code for localizing a definition that

> it's determined need not be externally visible.  It looks to me that the

> not-quite-the-sameness is erroneous, and this patch refactors that code

> into a common subroutine. If the differences need to be maintained

> (slight differences in when unique_name is updated and whether

> resolution is set to LDPR_PREVAILING_DEF_IRONLY), I think a flag to the

> new function would be best, rather than keep the duplicated code.

>

> booted & tested on x86_64-linux, ok?

>

> nathan



-- 
Nathan Sidwell
Jan Hubicka Jan. 17, 2017, 1:49 p.m. | #2
> This patch refactors the decl localizing that happens in

> function_and_variable_visibility.  It doesn't fix the bug I'm working on

> (that's next).

> 

> Both the FOR_EACH_FUNCTION and FOR_EACH_VARIABLE loops contain very similar,

> but not quite the same code for localizing a definition that it's determined

> need not be externally visible.  It looks to me that the

> not-quite-the-sameness is erroneous, and this patch refactors that code into

> a common subroutine. If the differences need to be maintained (slight

> differences in when unique_name is updated and whether resolution is set to

> LDPR_PREVAILING_DEF_IRONLY), I think a flag to the new function would be

> best, rather than keep the duplicated code.

> 

> booted & tested on x86_64-linux, ok?


OK,
the code has indeed grown into quite a mess over the years ;)

Thanks,
Honza
> 

> nathan

> -- 

> Nathan Sidwell


> 2017-01-06  Nathan Sidwell  <nathan@acm.org>

> 

> 	* ipa-visibility.c (localize_node): New function, broken out of ...

> 	(function_and_variable_visibility): Call it.

> 

> Index: ipa-visibility.c

> ===================================================================

> --- ipa-visibility.c	(revision 244159)

> +++ ipa-visibility.c	(working copy)

> @@ -529,6 +529,53 @@ optimize_weakref (symtab_node *node)

>    gcc_assert (node->alias);

>  }

>  

> +/* NODE is an externally visible definition, which we've discovered is

> +   not needed externally.  Make it local to this compilation.  */

> +

> +static void

> +localize_node (bool whole_program, symtab_node *node)

> +{

> +  gcc_assert (whole_program || in_lto_p || !TREE_PUBLIC (node->decl));

> +

> +  if (node->same_comdat_group && TREE_PUBLIC (node->decl))

> +    {

> +      for (symtab_node *next = node->same_comdat_group;

> +	   next != node; next = next->same_comdat_group)

> +	{

> +	  next->set_comdat_group (NULL);

> +	  if (!next->alias)

> +	    next->set_section (NULL);

> +	  if (!next->transparent_alias)

> +	    next->make_decl_local ();

> +	  next->unique_name

> +	    |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY

> +		 || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)

> +		&& TREE_PUBLIC (next->decl)

> +		&& !flag_incremental_link);

> +	}

> +

> +      /* Now everything's localized, the grouping has no meaning, and

> +	 will cause crashes if we keep it around.  */

> +      node->dissolve_same_comdat_group_list ();

> +    }

> +

> +  node->unique_name

> +    |= ((node->resolution == LDPR_PREVAILING_DEF_IRONLY

> +	 || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)

> +	&& TREE_PUBLIC (node->decl)

> +	&& !flag_incremental_link);

> +

> +  if (TREE_PUBLIC (node->decl))

> +    node->set_comdat_group (NULL);

> +  if (DECL_COMDAT (node->decl) && !node->alias)

> +    node->set_section (NULL);

> +  if (!node->transparent_alias)

> +    {

> +      node->resolution = LDPR_PREVAILING_DEF_IRONLY;

> +      node->make_decl_local ();

> +    }

> +}

> +

>  /* Decide on visibility of all symbols.  */

>  

>  static unsigned int

> @@ -606,48 +653,7 @@ function_and_variable_visibility (bool w

>        if (!node->externally_visible

>  	  && node->definition && !node->weakref

>  	  && !DECL_EXTERNAL (node->decl))

> -	{

> -	  gcc_assert (whole_program || in_lto_p

> -		      || !TREE_PUBLIC (node->decl));

> -	  node->unique_name

> -	    |= ((node->resolution == LDPR_PREVAILING_DEF_IRONLY

> -		 || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)

> -		&& TREE_PUBLIC (node->decl)

> -		&& !flag_incremental_link);

> -	  node->resolution = LDPR_PREVAILING_DEF_IRONLY;

> -	  if (node->same_comdat_group && TREE_PUBLIC (node->decl))

> -	    {

> -	      symtab_node *next = node;

> -

> -	      /* Set all members of comdat group local.  */

> -	      for (next = node->same_comdat_group;

> -		   next != node;

> -		   next = next->same_comdat_group)

> -		{

> -		  next->set_comdat_group (NULL);

> -		  if (!next->alias)

> -		    next->set_section (NULL);

> -		  if (!next->transparent_alias)

> -		    next->make_decl_local ();

> -		  next->unique_name

> -		    |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY

> -			 || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)

> -			&& TREE_PUBLIC (next->decl)

> -			&& !flag_incremental_link);

> -		}

> -	      /* cgraph_externally_visible_p has already checked all

> -	         other nodes in the group and they will all be made

> -	         local.  We need to dissolve the group at once so that

> -	         the predicate does not segfault though. */

> -	      node->dissolve_same_comdat_group_list ();

> -	    }

> -	  if (TREE_PUBLIC (node->decl))

> -	    node->set_comdat_group (NULL);

> -	  if (DECL_COMDAT (node->decl) && !node->alias)

> -	    node->set_section (NULL);

> -	  if (!node->transparent_alias)

> -	    node->make_decl_local ();

> -	}

> +	localize_node (whole_program, node);

>  

>        if (node->thunk.thunk_p

>  	  && !node->thunk.add_pointer_bounds_args

> @@ -757,49 +763,11 @@ function_and_variable_visibility (bool w

>        if (lookup_attribute ("no_reorder",

>  			    DECL_ATTRIBUTES (vnode->decl)))

>  	vnode->no_reorder = 1;

> +

>        if (!vnode->externally_visible

>  	  && !vnode->transparent_alias)

> -	{

> -	  gcc_assert (in_lto_p || whole_program || !TREE_PUBLIC (vnode->decl));

> -	  vnode->unique_name |= ((vnode->resolution == LDPR_PREVAILING_DEF_IRONLY

> -			          || vnode->resolution

> -				      == LDPR_PREVAILING_DEF_IRONLY_EXP)

> -			         && TREE_PUBLIC (vnode->decl)

> -				 && !flag_incremental_link);

> -	  if (vnode->same_comdat_group && TREE_PUBLIC (vnode->decl))

> -	    {

> -	      symtab_node *next = vnode;

> +	localize_node (whole_program, vnode);

>  

> -	      /* Set all members of comdat group local.  */

> -	      if (vnode->same_comdat_group)

> -		for (next = vnode->same_comdat_group;

> -		     next != vnode;

> -		     next = next->same_comdat_group)

> -		{

> -		  next->set_comdat_group (NULL);

> -		  if (!next->alias)

> -		    next->set_section (NULL);

> -		  if (!next->transparent_alias)

> -		    {

> -		      next->make_decl_local ();

> -		      next->unique_name |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY

> -					     || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)

> -					    && TREE_PUBLIC (next->decl)

> -					    && !flag_incremental_link);

> -		    }

> -		}

> -	      vnode->dissolve_same_comdat_group_list ();

> -	    }

> -	  if (TREE_PUBLIC (vnode->decl))

> -	    vnode->set_comdat_group (NULL);

> -	  if (DECL_COMDAT (vnode->decl) && !vnode->alias)

> -	    vnode->set_section (NULL);

> -	  if (!vnode->transparent_alias)

> -	    {

> -	      vnode->make_decl_local ();

> -	      vnode->resolution = LDPR_PREVAILING_DEF_IRONLY;

> -	    }

> -	}

>        update_visibility_by_resolution_info (vnode);

>  

>        /* Update virtual tables to point to local aliases where possible.  */

Patch hide | download patch | download mbox

2017-01-06  Nathan Sidwell  <nathan@acm.org>

	* ipa-visibility.c (localize_node): New function, broken out of ...
	(function_and_variable_visibility): Call it.

Index: ipa-visibility.c
===================================================================
--- ipa-visibility.c	(revision 244159)
+++ ipa-visibility.c	(working copy)
@@ -529,6 +529,53 @@  optimize_weakref (symtab_node *node)
   gcc_assert (node->alias);
 }
 
+/* NODE is an externally visible definition, which we've discovered is
+   not needed externally.  Make it local to this compilation.  */
+
+static void
+localize_node (bool whole_program, symtab_node *node)
+{
+  gcc_assert (whole_program || in_lto_p || !TREE_PUBLIC (node->decl));
+
+  if (node->same_comdat_group && TREE_PUBLIC (node->decl))
+    {
+      for (symtab_node *next = node->same_comdat_group;
+	   next != node; next = next->same_comdat_group)
+	{
+	  next->set_comdat_group (NULL);
+	  if (!next->alias)
+	    next->set_section (NULL);
+	  if (!next->transparent_alias)
+	    next->make_decl_local ();
+	  next->unique_name
+	    |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
+		 || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
+		&& TREE_PUBLIC (next->decl)
+		&& !flag_incremental_link);
+	}
+
+      /* Now everything's localized, the grouping has no meaning, and
+	 will cause crashes if we keep it around.  */
+      node->dissolve_same_comdat_group_list ();
+    }
+
+  node->unique_name
+    |= ((node->resolution == LDPR_PREVAILING_DEF_IRONLY
+	 || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
+	&& TREE_PUBLIC (node->decl)
+	&& !flag_incremental_link);
+
+  if (TREE_PUBLIC (node->decl))
+    node->set_comdat_group (NULL);
+  if (DECL_COMDAT (node->decl) && !node->alias)
+    node->set_section (NULL);
+  if (!node->transparent_alias)
+    {
+      node->resolution = LDPR_PREVAILING_DEF_IRONLY;
+      node->make_decl_local ();
+    }
+}
+
 /* Decide on visibility of all symbols.  */
 
 static unsigned int
@@ -606,48 +653,7 @@  function_and_variable_visibility (bool w
       if (!node->externally_visible
 	  && node->definition && !node->weakref
 	  && !DECL_EXTERNAL (node->decl))
-	{
-	  gcc_assert (whole_program || in_lto_p
-		      || !TREE_PUBLIC (node->decl));
-	  node->unique_name
-	    |= ((node->resolution == LDPR_PREVAILING_DEF_IRONLY
-		 || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
-		&& TREE_PUBLIC (node->decl)
-		&& !flag_incremental_link);
-	  node->resolution = LDPR_PREVAILING_DEF_IRONLY;
-	  if (node->same_comdat_group && TREE_PUBLIC (node->decl))
-	    {
-	      symtab_node *next = node;
-
-	      /* Set all members of comdat group local.  */
-	      for (next = node->same_comdat_group;
-		   next != node;
-		   next = next->same_comdat_group)
-		{
-		  next->set_comdat_group (NULL);
-		  if (!next->alias)
-		    next->set_section (NULL);
-		  if (!next->transparent_alias)
-		    next->make_decl_local ();
-		  next->unique_name
-		    |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
-			 || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
-			&& TREE_PUBLIC (next->decl)
-			&& !flag_incremental_link);
-		}
-	      /* cgraph_externally_visible_p has already checked all
-	         other nodes in the group and they will all be made
-	         local.  We need to dissolve the group at once so that
-	         the predicate does not segfault though. */
-	      node->dissolve_same_comdat_group_list ();
-	    }
-	  if (TREE_PUBLIC (node->decl))
-	    node->set_comdat_group (NULL);
-	  if (DECL_COMDAT (node->decl) && !node->alias)
-	    node->set_section (NULL);
-	  if (!node->transparent_alias)
-	    node->make_decl_local ();
-	}
+	localize_node (whole_program, node);
 
       if (node->thunk.thunk_p
 	  && !node->thunk.add_pointer_bounds_args
@@ -757,49 +763,11 @@  function_and_variable_visibility (bool w
       if (lookup_attribute ("no_reorder",
 			    DECL_ATTRIBUTES (vnode->decl)))
 	vnode->no_reorder = 1;
+
       if (!vnode->externally_visible
 	  && !vnode->transparent_alias)
-	{
-	  gcc_assert (in_lto_p || whole_program || !TREE_PUBLIC (vnode->decl));
-	  vnode->unique_name |= ((vnode->resolution == LDPR_PREVAILING_DEF_IRONLY
-			          || vnode->resolution
-				      == LDPR_PREVAILING_DEF_IRONLY_EXP)
-			         && TREE_PUBLIC (vnode->decl)
-				 && !flag_incremental_link);
-	  if (vnode->same_comdat_group && TREE_PUBLIC (vnode->decl))
-	    {
-	      symtab_node *next = vnode;
+	localize_node (whole_program, vnode);
 
-	      /* Set all members of comdat group local.  */
-	      if (vnode->same_comdat_group)
-		for (next = vnode->same_comdat_group;
-		     next != vnode;
-		     next = next->same_comdat_group)
-		{
-		  next->set_comdat_group (NULL);
-		  if (!next->alias)
-		    next->set_section (NULL);
-		  if (!next->transparent_alias)
-		    {
-		      next->make_decl_local ();
-		      next->unique_name |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
-					     || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
-					    && TREE_PUBLIC (next->decl)
-					    && !flag_incremental_link);
-		    }
-		}
-	      vnode->dissolve_same_comdat_group_list ();
-	    }
-	  if (TREE_PUBLIC (vnode->decl))
-	    vnode->set_comdat_group (NULL);
-	  if (DECL_COMDAT (vnode->decl) && !vnode->alias)
-	    vnode->set_section (NULL);
-	  if (!vnode->transparent_alias)
-	    {
-	      vnode->make_decl_local ();
-	      vnode->resolution = LDPR_PREVAILING_DEF_IRONLY;
-	    }
-	}
       update_visibility_by_resolution_info (vnode);
 
       /* Update virtual tables to point to local aliases where possible.  */