Re: [PATCH, PR 42450] Consistent call statement redirection for inline clones

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Thu, Mar 11, 2010 at 05:44:17PM +0100, Martin Jambor wrote:
> On Wed, Mar 10, 2010 at 04:57:34PM +0100, Jan Hubicka wrote:
> > > The problem in PR 42450 is slightly different from what I saw when
> > > writing the patch.  The core problem is that an inline clone has three
> > > outgoing edges, whereas its clone_of node has only two.  The third one
> > > is removed along with the node it leads to by
> > > cgraph_remove_unreachable_nodes.  The caller isn't removed but it is
> > 
> > Hmm, this is bad situation :(
> > We keep the edges around in order to make tree-inline update call edges of
> > the clones for new statements when they are created.  If some of them gets
> > removed because the callee is unreachable, we run into problem.
> 
> I tried to find such updates in tree-inline but couldn't.  As far as I
> know, the edges that are actually used... in fact, all edges always
> point to the right places at all times (at least after each IPA pass
> is finished).  It's the statements that need to be updated according
> to them.
> 
> I don't thing this is something really bad.  In fact, instead I would
> propose that we delete all outgoing edges of such unreachable nodes
> that are kept around only to hold bodies so that they don't confuse
> us.  But I assume that is what is done by the following:
> 
> 
> > Does the patch work if you move cgraph_node_remove_callees out of
> > the following conditional in ipa.c?
> >                   if (!clone)
> >                     { 
> >                       cgraph_release_function_body (node);
> >                       cgraph_node_remove_callees (node);
> >                       node->analyzed = false;
> >                       node->local.inlinable = false;
> >                     }
> > 
> 
> Yes, when I move the call from the if block, the PR is still not
> present and there are no regressions among c, c++ and libstc++
> testsuits.  I will do a full bootstrap and test round soon.
> 

This is the updated patch.  It passes all+ada bootstrap and testsuite
without any regressions.  What do you think?

Thanks,

Martin

2010-03-11  Martin Jambor  <mjambor@suse.cz>

	PR middle-end/42450
	* cgraph.h (cgraph_redirect_edge_call_stmt_to_callee): Declare.
	* cgraphunit.c (cgraph_materialize_all_clones): Update calls in
	all non-clones.  Moved call redirection...
	(cgraph_redirect_edge_call_stmt_to_callee): ...to this new
	function.
	(cgraph_materialize_all_clones): Dispose of all
	combined_args_to_skip bitmaps.
	(verify_cgraph_node): Do not check for edges pointing to wrong
	nodes in inline clones.
	* tree-inline.c (copy_bb): Call
	cgraph_redirect_edge_call_stmt_to_callee.
	* ipa.c (cgraph_remove_unreachable_nodes): Call
	cgraph_node_remove_callees even when there are used clones.

	* testsuite/g++.dg/torture/pr42450.C: New test.


Index: icln/gcc/cgraph.h
===================================================================
--- icln.orig/gcc/cgraph.h
+++ icln/gcc/cgraph.h
@@ -534,7 +534,7 @@ void cgraph_remove_edge_duplication_hook
 struct cgraph_2node_hook_list *cgraph_add_node_duplication_hook (cgraph_2node_hook, void *);
 void cgraph_remove_node_duplication_hook (struct cgraph_2node_hook_list *);
 void cgraph_materialize_all_clones (void);
-
+gimple cgraph_redirect_edge_call_stmt_to_callee (struct cgraph_edge *);
 /* In cgraphbuild.c  */
 unsigned int rebuild_cgraph_edges (void);
 void reset_inline_failed (struct cgraph_node *);
Index: icln/gcc/cgraphunit.c
===================================================================
--- icln.orig/gcc/cgraphunit.c
+++ icln/gcc/cgraphunit.c
@@ -751,8 +751,9 @@ verify_cgraph_node (struct cgraph_node *
 			    debug_tree (e->callee->decl);
 			    error_found = true;
 			  }
-			else if (!clone_of_p (cgraph_node (decl), e->callee)
-			         && !e->callee->global.inlined_to)
+			else if (!node->global.inlined_to
+				 && !e->callee->global.inlined_to
+				 && !clone_of_p (cgraph_node (decl), e->callee))
 			  {
 			    error ("edge points to wrong declaration:");
 			    debug_tree (e->callee->decl);
@@ -2222,11 +2223,60 @@ cgraph_materialize_clone (struct cgraph_
   bitmap_obstack_release (NULL);
 }
 
+/* If necessary, change the function declaration in the call statement
+   associated with E so that it corresponds to the edge callee.  */
+
+gimple
+cgraph_redirect_edge_call_stmt_to_callee (struct cgraph_edge *e)
+{
+  tree decl = gimple_call_fndecl (e->call_stmt);
+  gimple new_stmt;
+  gimple_stmt_iterator gsi;
+
+  if (!decl || decl == e->callee->decl
+      /* Don't update call from same body alias to the real function.  */
+      || cgraph_get_node (decl) == cgraph_get_node (e->callee->decl))
+    return e->call_stmt;
+
+  if (cgraph_dump_file)
+    {
+      fprintf (cgraph_dump_file, "updating call of %s/%i -> %s/%i: ",
+	       cgraph_node_name (e->caller), e->caller->uid,
+	       cgraph_node_name (e->callee), e->callee->uid);
+      print_gimple_stmt (cgraph_dump_file, e->call_stmt, 0, dump_flags);
+    }
+
+  if (e->callee->clone.combined_args_to_skip)
+    new_stmt = gimple_call_copy_skip_args (e->call_stmt,
+				       e->callee->clone.combined_args_to_skip);
+  else
+    new_stmt = e->call_stmt;
+  if (gimple_vdef (new_stmt)
+      && TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME)
+    SSA_NAME_DEF_STMT (gimple_vdef (new_stmt)) = new_stmt;
+  gimple_call_set_fndecl (new_stmt, e->callee->decl);
+
+  gsi = gsi_for_stmt (e->call_stmt);
+  gsi_replace (&gsi, new_stmt, true);
+
+  /* Update EH information too, just in case.  */
+  maybe_clean_or_replace_eh_stmt (e->call_stmt, new_stmt);
+
+  cgraph_set_call_stmt_including_clones (e->caller, e->call_stmt, new_stmt);
+
+  if (cgraph_dump_file)
+    {
+      fprintf (cgraph_dump_file, "  updated to:");
+      print_gimple_stmt (cgraph_dump_file, e->call_stmt, 0, dump_flags);
+    }
+  return new_stmt;
+}
+
 /* Once all functions from compilation unit are in memory, produce all clones
-   and update all calls.
-   We might also do this on demand if we don't want to bring all functions to
-   memory prior compilation, but current WHOPR implementation does that and it is
-   is bit easier to keep everything right in this order.  */
+   and update all calls.  We might also do this on demand if we don't want to
+   bring all functions to memory prior compilation, but current WHOPR
+   implementation does that and it is is bit easier to keep everything right in
+   this order.  */
 void
 cgraph_materialize_all_clones (void)
 {
@@ -2302,69 +2352,28 @@ cgraph_materialize_all_clones (void)
   if (cgraph_dump_file)
     fprintf (cgraph_dump_file, "Updating call sites\n");
   for (node = cgraph_nodes; node; node = node->next)
-    if (node->analyzed && gimple_has_body_p (node->decl)
-        && (!node->clone_of || node->clone_of->decl != node->decl))
+    if (node->analyzed && !node->clone_of
+	&& gimple_has_body_p (node->decl))
       {
         struct cgraph_edge *e;
 
 	current_function_decl = node->decl;
         push_cfun (DECL_STRUCT_FUNCTION (node->decl));
 	for (e = node->callees; e; e = e->next_callee)
-	  {
-	    tree decl = gimple_call_fndecl (e->call_stmt);
-	    /* When function gets inlined, indirect inlining might've invented
-	       new edge for orginally indirect stmt.  Since we are not
-	       preserving clones in the original form, we must not update here
-	       since other inline clones don't need to contain call to the same
-	       call.  Inliner will do the substitution for us later.  */
-	    if (decl && decl != e->callee->decl)
-	      {
-		gimple new_stmt;
-		gimple_stmt_iterator gsi;
-
-		if (cgraph_get_node (decl) == cgraph_get_node (e->callee->decl))
-		  /* Don't update call from same body alias to the real function.  */
-		  continue;
-
-		if (cgraph_dump_file)
-		  {
-		    fprintf (cgraph_dump_file, "updating call of %s in %s:",
-		             cgraph_node_name (node),
-			     cgraph_node_name (e->callee));
-      		    print_gimple_stmt (cgraph_dump_file, e->call_stmt, 0, dump_flags);
-		  }
-
-		if (e->callee->clone.combined_args_to_skip)
-		  new_stmt = gimple_call_copy_skip_args (e->call_stmt,
-							 e->callee->clone.combined_args_to_skip);
-		else
-		  new_stmt = e->call_stmt;
-		if (gimple_vdef (new_stmt)
-		    && TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME)
-		  SSA_NAME_DEF_STMT (gimple_vdef (new_stmt)) = new_stmt;
-                gimple_call_set_fndecl (new_stmt, e->callee->decl);
-
-		gsi = gsi_for_stmt (e->call_stmt);
-		gsi_replace (&gsi, new_stmt, true);
-
-		/* Update EH information too, just in case.  */
-		maybe_clean_or_replace_eh_stmt (e->call_stmt, new_stmt);
-
-		cgraph_set_call_stmt_including_clones (node, e->call_stmt, new_stmt);
-
-		if (cgraph_dump_file)
-		  {
-		    fprintf (cgraph_dump_file, "  updated to:");
-      		    print_gimple_stmt (cgraph_dump_file, e->call_stmt, 0, dump_flags);
-		  }
-	      }
-	  }
+	  cgraph_redirect_edge_call_stmt_to_callee (e);
 	pop_cfun ();
 	current_function_decl = NULL;
 #ifdef ENABLE_CHECKING
         verify_cgraph_node (node);
 #endif
       }
+  if (cgraph_dump_file)
+    fprintf (cgraph_dump_file, "Materialization Call site updates done.\n");
+  /* All changes to parameters have been performed.  In order not to
+     incorrectly repeat them, we simply dispose of the bitmaps that drive the
+     changes. */
+  for (node = cgraph_nodes; node; node = node->next)
+    node->clone.combined_args_to_skip = NULL;
 #ifdef ENABLE_CHECKING
   verify_cgraph ();
 #endif
Index: icln/gcc/tree-inline.c
===================================================================
--- icln.orig/gcc/tree-inline.c
+++ icln/gcc/tree-inline.c
@@ -1650,6 +1650,7 @@ copy_bb (copy_body_data *id, basic_block
 				   bb->frequency,
 				   copy_basic_block->frequency);
 			}
+		      stmt = cgraph_redirect_edge_call_stmt_to_callee (edge);
 		    }
 		  break;
 
Index: icln/gcc/testsuite/g++.dg/torture/pr42450.C
===================================================================
--- /dev/null
+++ icln/gcc/testsuite/g++.dg/torture/pr42450.C
@@ -0,0 +1,112 @@
+/* { dg-do compile } */
+
+template < typename > class basic_stringstream;
+
+struct basic_string {
+  basic_string();
+};
+
+struct ios_base {
+  virtual ~ios_base();
+};
+
+class ostream:ios_base {};
+class istream:virtual ios_base {};
+
+template < typename > struct basic_iostream:public istream, ostream {
+  ~basic_iostream () {}
+};
+extern template class basic_iostream < char >;
+
+template < typename > struct basic_stringstream:public basic_iostream < char > {
+    basic_string _M_stringbuf;
+    ~basic_stringstream () {}
+};
+extern template class basic_stringstream < char >;
+
+template < typename > struct AnyMatrixBase;
+template < typename, int _Rows, int _Cols, int = _Rows, int = _Cols > class Matrix;
+template < typename > class CwiseNullaryOp;
+
+template < typename Derived > struct MatrixBase:public AnyMatrixBase < Derived > {
+  typedef CwiseNullaryOp < Derived > ConstantReturnType;
+  ConstantReturnType Constant ();
+  template < typename > Derived cast ();
+  static CwiseNullaryOp < Derived > Random (int);
+};
+
+template < typename Derived > struct AnyMatrixBase {
+  Derived derived () {}
+  Derived & derived () const {}
+};
+
+template < typename, int > struct ei_matrix_storage {};
+
+template < typename _Scalar, int, int, int _MaxRows, int _MaxCols > struct Matrix:MatrixBase < Matrix < _Scalar, _MaxRows, _MaxCols > > {
+  typedef MatrixBase < Matrix > Base;
+  ei_matrix_storage < int, _MaxCols > m_storage;
+  Matrix operator= (const Matrix other) {
+    _resize_to_match (other);
+    lazyAssign (other.derived ());
+  }
+  template < typename OtherDerived > Matrix lazyAssign (MatrixBase < OtherDerived > other) {
+    _resize_to_match (other);
+    return Base (other.derived ());
+  }
+  Matrix ();
+  template < typename OtherDerived > Matrix (const MatrixBase < OtherDerived > &other) {
+    *this = other;
+  }
+  template < typename OtherDerived > void _resize_to_match (const MatrixBase < OtherDerived > &) {
+    throw 1;
+  }
+};
+
+template < typename MatrixType > class CwiseNullaryOp:
+public MatrixBase < CwiseNullaryOp < MatrixType > > {};
+
+int f()
+{
+  bool align_cols;
+  if (align_cols) {
+    basic_stringstream<char> sstr;
+    f();
+  }
+}
+
+template < typename > struct AutoDiffScalar;
+template < typename Functor > struct AutoDiffJacobian:Functor {
+  AutoDiffJacobian (Functor);
+  typedef typename Functor::InputType InputType;
+  typedef typename Functor::ValueType ValueType;
+  typedef Matrix < int, Functor::InputsAtCompileTime, 1 > DerivativeType;
+  typedef AutoDiffScalar < DerivativeType > ActiveScalar;
+  typedef Matrix < ActiveScalar, Functor::InputsAtCompileTime, 1 > ActiveInput;
+  void operator () (InputType x, ValueType *) {
+    ActiveInput ax = x.template cast < ActiveScalar > ();
+  }
+};
+
+template < int NX, int NY > struct TestFunc1 {
+  enum  {
+    InputsAtCompileTime = NX
+  };
+  typedef Matrix < float, NX, 1 > InputType;
+  typedef Matrix < float, NY, 1 > ValueType;
+  typedef Matrix < float, NY, NX > JacobianType;
+  int inputs ();
+};
+
+template < typename Func > void forward_jacobian (Func f) {
+  typename Func::InputType x = Func::InputType::Random (f.inputs ());
+  typename Func::ValueType y;
+  typename Func::JacobianType jref = jref.Constant ();
+  AutoDiffJacobian < Func > autoj (f);
+  autoj (x, &y);
+}
+
+void test_autodiff_scalar ()
+{
+  forward_jacobian (TestFunc1 < 2, 2 > ());
+  forward_jacobian (TestFunc1 < 3, 2 > ());
+}
Index: icln/gcc/ipa.c
===================================================================
--- icln.orig/gcc/ipa.c
+++ icln/gcc/ipa.c
@@ -262,10 +262,10 @@ cgraph_remove_unreachable_nodes (bool be
 		  if (!clone)
 		    {
 		      cgraph_release_function_body (node);
-		      cgraph_node_remove_callees (node);
 		      node->analyzed = false;
 		      node->local.inlinable = false;
 		    }
+		  cgraph_node_remove_callees (node);
 		  if (node->prev_sibling_clone)
 		    node->prev_sibling_clone->next_sibling_clone = node->next_sibling_clone;
 		  else if (node->clone_of)