ipa: Prevent materialization of clones with removed bodies (PR 92109)
diff mbox series

Message ID ri6y2w7g9lp.fsf@suse.cz
State New
Headers show
Series
  • ipa: Prevent materialization of clones with removed bodies (PR 92109)
Related show

Commit Message

Martin Jambor Nov. 22, 2019, 4:27 p.m. UTC
Hi,

this bug is an interesting one.  The immediate cause of the ICE is that
IPA-SRA modification phase baked into clone materialization was
expecting to see a body already changed by IPA-CP while it was actually
looking at the original function.

That was because we were materializing the (offline) clone in an ltrans
from which it was called but where it did not belonged at all.
remove_unreachable_nodes realized this and wanted to keep just the
declaration and remove the body - and therefor also all traces of the
IPA-CP clone which was necessary just for materialization.  But
materialization code still attempted to resurrect the clone.

Honza proposed immediately removing any node which only needs to have a
declaration from the tree of clones to prevent materialization code from
even considering them which is what the following patch does.

Unfortunately, I was not able to come up with a small testcase, the bug
is triggered by fragile partitioning decisions.  I have bootstrapped,
LTO-bootstrapped and tested the patch on an x86_64-linux, OK for trunk?

Thanks,

Martin



2019-11-21  Martin Jambor  <mjambor@suse.cz>

	PR ipa/92109
	* cgraph.h (cgraph_node::remove_from_clone_tree): Declare.
	* cgraphclones.c (cgraph_node::remove_from_clone_tree): New method.
	(cgraph_materialize_clone): Move removel from clone tree to the
	the new method and use it instead.
	* ipa.c (symbol_table::remove_unreachable_nodes): When removing
	bodies of clones, also remove it from the clone tree.
---
 gcc/cgraph.h       |  4 ++++
 gcc/cgraphclones.c | 35 ++++++++++++++++++++++-------------
 gcc/ipa.c          | 11 ++++++++---
 3 files changed, 34 insertions(+), 16 deletions(-)

Comments

Jan Hubicka Nov. 22, 2019, 4:57 p.m. UTC | #1
> Hi,
> 
> this bug is an interesting one.  The immediate cause of the ICE is that
> IPA-SRA modification phase baked into clone materialization was
> expecting to see a body already changed by IPA-CP while it was actually
> looking at the original function.
> 
> That was because we were materializing the (offline) clone in an ltrans
> from which it was called but where it did not belonged at all.
> remove_unreachable_nodes realized this and wanted to keep just the
> declaration and remove the body - and therefor also all traces of the
> IPA-CP clone which was necessary just for materialization.  But
> materialization code still attempted to resurrect the clone.
> 
> Honza proposed immediately removing any node which only needs to have a
> declaration from the tree of clones to prevent materialization code from
> even considering them which is what the following patch does.
> 
> Unfortunately, I was not able to come up with a small testcase, the bug
> is triggered by fragile partitioning decisions.  I have bootstrapped,
> LTO-bootstrapped and tested the patch on an x86_64-linux, OK for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 
> 2019-11-21  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/92109
> 	* cgraph.h (cgraph_node::remove_from_clone_tree): Declare.
> 	* cgraphclones.c (cgraph_node::remove_from_clone_tree): New method.
> 	(cgraph_materialize_clone): Move removel from clone tree to the
> 	the new method and use it instead.
> 	* ipa.c (symbol_table::remove_unreachable_nodes): When removing
> 	bodies of clones, also remove it from the clone tree.

OK,
thanks!
Honza
> ---
>  gcc/cgraph.h       |  4 ++++
>  gcc/cgraphclones.c | 35 ++++++++++++++++++++++-------------
>  gcc/ipa.c          | 11 ++++++++---
>  3 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index a4f14743f00..0d2442c997c 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -967,6 +967,10 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
>  				     ipa_param_adjustments *param_adjustments,
>  				     const char * suffix, unsigned num_suffix);
>  
> +  /* Remove the node from the tree of virtual and inline clones and make it a
> +     standalone node - not a clone any more.  */
> +  void remove_from_clone_tree ();
> +
>    /* cgraph node being removed from symbol table; see if its entry can be
>     replaced by other inline clone.  */
>    cgraph_node *find_replacement (void);
> diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
> index bfcebb20495..ac5c57a47aa 100644
> --- a/gcc/cgraphclones.c
> +++ b/gcc/cgraphclones.c
> @@ -1013,6 +1013,22 @@ cgraph_node::create_version_clone_with_body
>    return new_version_node;
>  }
>  
> +/* Remove the node from the tree of virtual and inline clones and make it a
> +   standalone node - not a clone any more.  */
> +
> +void cgraph_node::remove_from_clone_tree ()
> +{
> +  if (next_sibling_clone)
> +    next_sibling_clone->prev_sibling_clone = prev_sibling_clone;
> +  if (prev_sibling_clone)
> +    prev_sibling_clone->next_sibling_clone = next_sibling_clone;
> +  else
> +    clone_of->clones = next_sibling_clone;
> +  next_sibling_clone = NULL;
> +  prev_sibling_clone = NULL;
> +  clone_of = NULL;
> +}
> +
>  /* Given virtual clone, turn it into actual clone.  */
>  
>  static void
> @@ -1033,22 +1049,15 @@ cgraph_materialize_clone (cgraph_node *node)
>        dump_function_to_file (node->decl, symtab->dump_file, dump_flags);
>      }
>  
> +  cgraph_node *clone_of = node->clone_of;
>    /* Function is no longer clone.  */
> -  if (node->next_sibling_clone)
> -    node->next_sibling_clone->prev_sibling_clone = node->prev_sibling_clone;
> -  if (node->prev_sibling_clone)
> -    node->prev_sibling_clone->next_sibling_clone = node->next_sibling_clone;
> -  else
> -    node->clone_of->clones = node->next_sibling_clone;
> -  node->next_sibling_clone = NULL;
> -  node->prev_sibling_clone = NULL;
> -  if (!node->clone_of->analyzed && !node->clone_of->clones)
> +  node->remove_from_clone_tree ();
> +  if (!clone_of->analyzed && !clone_of->clones)
>      {
> -      node->clone_of->release_body ();
> -      node->clone_of->remove_callees ();
> -      node->clone_of->remove_all_references ();
> +      clone_of->release_body ();
> +      clone_of->remove_callees ();
> +      clone_of->remove_all_references ();
>      }
> -  node->clone_of = NULL;
>    bitmap_obstack_release (NULL);
>  }
>  
> diff --git a/gcc/ipa.c b/gcc/ipa.c
> index 0c92980db46..2404024d722 100644
> --- a/gcc/ipa.c
> +++ b/gcc/ipa.c
> @@ -520,9 +520,14 @@ symbol_table::remove_unreachable_nodes (FILE *file)
>  	     reliably.  */
>  	  if (node->alias || node->thunk.thunk_p)
>  	    ;
> -	  else if (!body_needed_for_clonning.contains (node->decl)
> -	      && !node->alias && !node->thunk.thunk_p)
> -	    node->release_body ();
> +	  else if (!body_needed_for_clonning.contains (node->decl))
> +	    {
> +	      /* Make the node a non-clone so that we do not attempt to
> +		 materialize it later.  */
> +	      if (node->clone_of)
> +		node->remove_from_clone_tree ();
> +	      node->release_body ();
> +	    }
>  	  else if (!node->clone_of)
>  	    gcc_assert (in_lto_p || DECL_RESULT (node->decl));
>  	  if (node->definition && !node->alias && !node->thunk.thunk_p)
> -- 
> 2.23.0
>

Patch
diff mbox series

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index a4f14743f00..0d2442c997c 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -967,6 +967,10 @@  struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
 				     ipa_param_adjustments *param_adjustments,
 				     const char * suffix, unsigned num_suffix);
 
+  /* Remove the node from the tree of virtual and inline clones and make it a
+     standalone node - not a clone any more.  */
+  void remove_from_clone_tree ();
+
   /* cgraph node being removed from symbol table; see if its entry can be
    replaced by other inline clone.  */
   cgraph_node *find_replacement (void);
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index bfcebb20495..ac5c57a47aa 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -1013,6 +1013,22 @@  cgraph_node::create_version_clone_with_body
   return new_version_node;
 }
 
+/* Remove the node from the tree of virtual and inline clones and make it a
+   standalone node - not a clone any more.  */
+
+void cgraph_node::remove_from_clone_tree ()
+{
+  if (next_sibling_clone)
+    next_sibling_clone->prev_sibling_clone = prev_sibling_clone;
+  if (prev_sibling_clone)
+    prev_sibling_clone->next_sibling_clone = next_sibling_clone;
+  else
+    clone_of->clones = next_sibling_clone;
+  next_sibling_clone = NULL;
+  prev_sibling_clone = NULL;
+  clone_of = NULL;
+}
+
 /* Given virtual clone, turn it into actual clone.  */
 
 static void
@@ -1033,22 +1049,15 @@  cgraph_materialize_clone (cgraph_node *node)
       dump_function_to_file (node->decl, symtab->dump_file, dump_flags);
     }
 
+  cgraph_node *clone_of = node->clone_of;
   /* Function is no longer clone.  */
-  if (node->next_sibling_clone)
-    node->next_sibling_clone->prev_sibling_clone = node->prev_sibling_clone;
-  if (node->prev_sibling_clone)
-    node->prev_sibling_clone->next_sibling_clone = node->next_sibling_clone;
-  else
-    node->clone_of->clones = node->next_sibling_clone;
-  node->next_sibling_clone = NULL;
-  node->prev_sibling_clone = NULL;
-  if (!node->clone_of->analyzed && !node->clone_of->clones)
+  node->remove_from_clone_tree ();
+  if (!clone_of->analyzed && !clone_of->clones)
     {
-      node->clone_of->release_body ();
-      node->clone_of->remove_callees ();
-      node->clone_of->remove_all_references ();
+      clone_of->release_body ();
+      clone_of->remove_callees ();
+      clone_of->remove_all_references ();
     }
-  node->clone_of = NULL;
   bitmap_obstack_release (NULL);
 }
 
diff --git a/gcc/ipa.c b/gcc/ipa.c
index 0c92980db46..2404024d722 100644
--- a/gcc/ipa.c
+++ b/gcc/ipa.c
@@ -520,9 +520,14 @@  symbol_table::remove_unreachable_nodes (FILE *file)
 	     reliably.  */
 	  if (node->alias || node->thunk.thunk_p)
 	    ;
-	  else if (!body_needed_for_clonning.contains (node->decl)
-	      && !node->alias && !node->thunk.thunk_p)
-	    node->release_body ();
+	  else if (!body_needed_for_clonning.contains (node->decl))
+	    {
+	      /* Make the node a non-clone so that we do not attempt to
+		 materialize it later.  */
+	      if (node->clone_of)
+		node->remove_from_clone_tree ();
+	      node->release_body ();
+	    }
 	  else if (!node->clone_of)
 	    gcc_assert (in_lto_p || DECL_RESULT (node->decl));
 	  if (node->definition && !node->alias && !node->thunk.thunk_p)