Fix two issues with multi-target speculative calls

Message ID 20200119104641.GB15937@kam.mff.cuni.cz
State New
Headers show
Series
  • Fix two issues with multi-target speculative calls
Related show

Commit Message

Jan Hubicka Jan. 19, 2020, 10:46 a.m.
Hi,
this fixes two issues with the new multi-target speculation code which reproduce
on Firefox.  I can now build firefox with FDO locally but on Mozilla build bots
it still fails with ICE in speculative_call_info.

One problem is that speuclative code compares call_stmt and lto_stmt_uid in
a way that may get unwanted effect when these gets out of sync.  It does not
make sense to have both non-zero so I added code clearing it and sanity check
that it is kept this way.

Other problem is cgraph_edge::make_direct not working well with multiple
targets.  In this case it removed one speuclative target and the indirect call
leaving other targets in the tree.

This is fixed by iterating across all targets and removing all except the good
one (if it exists).

I will see if I can reproduce the other issue.

lto-bootstrapped/regtested x86_64-linux. I plan to commit it after bit of extra
testing.

2020-01-19  Jan Hubicka  <hubicka@ucw.cz>

	PR lto/93318
	* cgraph.c (cgraph_edge::resolve_speculation): Fix foramting.
	(cgraph_edge::make_direct): Remove all indirect targets.
	(cgraph_edge::redirect_call_stmt_to_callee): Use make_direct..
	(cgraph_node::verify_node): Verify that only one call_stmt or
	lto_stmt_uid is set.
	* cgraphclones.c (cgraph_edge::clone): Set only one call_stmt or
	lto_stmt_uid.
	* lto-cgraph.c (lto_output_edge): Simplify streaming of stmt.
	(lto_output_ref): Simplify streaming of stmt.
	* lto-streamer-in.c (fixup_call_stmt_edges_1): Clear lto_stmt_uid.

Comments

Jan Hubicka Jan. 19, 2020, 12:52 p.m. | #1
> Hi,

> this fixes two issues with the new multi-target speculation code which reproduce

> on Firefox.  I can now build firefox with FDO locally but on Mozilla build bots

> it still fails with ICE in speculative_call_info.

> 

> One problem is that speuclative code compares call_stmt and lto_stmt_uid in

> a way that may get unwanted effect when these gets out of sync.  It does not

> make sense to have both non-zero so I added code clearing it and sanity check

> that it is kept this way.

> 

> Other problem is cgraph_edge::make_direct not working well with multiple

> targets.  In this case it removed one speuclative target and the indirect call

> leaving other targets in the tree.

> 

> This is fixed by iterating across all targets and removing all except the good

> one (if it exists).

> 

> I will see if I can reproduce the other issue.

> 

> lto-bootstrapped/regtested x86_64-linux. I plan to commit it after bit of extra

> testing.

> 

> 2020-01-19  Jan Hubicka  <hubicka@ucw.cz>

> 

> 	PR lto/93318

> 	* cgraph.c (cgraph_edge::resolve_speculation): Fix foramting.

> 	(cgraph_edge::make_direct): Remove all indirect targets.

> 	(cgraph_edge::redirect_call_stmt_to_callee): Use make_direct..

> 	(cgraph_node::verify_node): Verify that only one call_stmt or

> 	lto_stmt_uid is set.

> 	* cgraphclones.c (cgraph_edge::clone): Set only one call_stmt or

> 	lto_stmt_uid.

> 	* lto-cgraph.c (lto_output_edge): Simplify streaming of stmt.

> 	(lto_output_ref): Simplify streaming of stmt.

> 	* lto-streamer-in.c (fixup_call_stmt_edges_1): Clear lto_stmt_uid.


Hi,
this is variant of patch I comitted.  There was one wrong sanity check
triggering ICE when speculation was resolved to different target then
predicted.

	PR lto/93318
	* cgraph.c (cgraph_edge::resolve_speculation): Fix foramting.
	(cgraph_edge::make_direct): Remove all indirect targets.
	(cgraph_edge::redirect_call_stmt_to_callee): Use make_direct..
	(cgraph_node::verify_node): Verify that only one call_stmt or
	lto_stmt_uid is set.
	* cgraphclones.c (cgraph_edge::clone): Set only one call_stmt or
	lto_stmt_uid.
	* lto-cgraph.c (lto_output_edge): Simplify streaming of stmt.
	(lto_output_ref): Simplify streaming of stmt.
	* lto-streamer-in.c (fixup_call_stmt_edges_1): Clear lto_stmt_uid.

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 95b523d6be5..c442f334fe2 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1226,8 +1226,8 @@ cgraph_edge::resolve_speculation (cgraph_edge *edge, tree callee_decl)
         fprintf (dump_file, "Speculative call turned into direct call.\n");
       edge = e2;
       e2 = tmp;
-      /* FIXME:  If EDGE is inlined, we should scale up the frequencies and counts
-         in the functions inlined through it.  */
+      /* FIXME:  If EDGE is inlined, we should scale up the frequencies
+	 and counts in the functions inlined through it.  */
     }
   edge->count += e2->count;
   if (edge->num_speculative_call_targets_p ())
@@ -1263,11 +1263,52 @@ cgraph_edge::make_direct (cgraph_edge *edge, cgraph_node *callee)
   /* If we are redirecting speculative call, make it non-speculative.  */
   if (edge->speculative)
     {
-      edge = resolve_speculation (edge, callee->decl);
+      cgraph_edge *found = NULL;
+      cgraph_edge *direct, *next;
+      ipa_ref *ref;
+
+      edge->speculative_call_info (direct, edge, ref);
 
-      /* On successful speculation just return the pre existing direct edge.  */
-      if (!edge->indirect_unknown_callee)
-        return edge;
+      /* Look all speculative targets and remove all but one corresponding
+	 to callee (if it exists).
+	 If there is only one target we can save one extra call to
+	 speculative_call_info.  */
+      if (edge->num_speculative_call_targets_p () != 1)
+	for (direct = edge->caller->callees; direct; direct = next)
+	  {
+	    next = direct->next_callee;
+	    if (direct->call_stmt == edge->call_stmt
+		&& direct->lto_stmt_uid == edge->lto_stmt_uid)
+	      {
+		direct->speculative_call_info (direct, edge, ref);
+
+		/* Compare ref not direct->callee.  Direct edge is possibly
+		   inlined or redirected.  */
+		if (!ref->referred->semantically_equivalent_p (callee))
+		  edge = direct->resolve_speculation (direct, NULL);
+		else
+		  {
+		    gcc_checking_assert (!found);
+		    found = direct;
+		  }
+	      }
+	  }
+	else if (!ref->referred->semantically_equivalent_p (callee))
+	  edge = direct->resolve_speculation (direct, NULL);
+	else
+	  found = direct;
+
+      /* On successful speculation just remove the indirect edge and
+	 return the pre existing direct edge.
+	 It is important to not remove it and redirect because the direct
+	 edge may be inlined or redirected.  */
+      if (found)
+	{
+	  resolve_speculation (edge, callee->decl);
+	  gcc_checking_assert (!found->speculative);
+	  return found;
+	}
+      gcc_checking_assert (!edge->speculative);
     }
 
   edge->indirect_unknown_callee = 0;
@@ -1328,7 +1369,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
       /* If there already is an direct call (i.e. as a result of inliner's
 	 substitution), forget about speculating.  */
       if (decl)
-	e = resolve_speculation (e, decl);
+	e = make_direct (e, cgraph_node::get (decl));
       else
 	{
 	  /* Expand speculation into GIMPLE code.  */
@@ -3116,6 +3157,8 @@ cgraph_node::verify_node (void)
   basic_block this_block;
   gimple_stmt_iterator gsi;
   bool error_found = false;
+  int i;
+  ipa_ref *ref = NULL;
 
   if (seen_error ())
     return;
@@ -3201,6 +3244,11 @@ cgraph_node::verify_node (void)
 	  cgraph_debug_gimple_stmt (this_cfun, e->call_stmt);
 	  error_found = true;
 	}
+      if (e->call_stmt && e->lto_stmt_uid)
+	{
+	  error ("edge has both cal_stmt and lto_stmt_uid set");
+	  error_found = true;
+	}
     }
   bool check_comdat = comdat_local_p ();
   for (e = callers; e; e = e->next_caller)
@@ -3267,6 +3315,11 @@ cgraph_node::verify_node (void)
 	  fprintf (stderr, "\n");
 	  error_found = true;
 	}
+      if (e->call_stmt && e->lto_stmt_uid)
+	{
+	  error ("edge has both cal_stmt and lto_stmt_uid set");
+	  error_found = true;
+	}
     }
   for (e = indirect_calls; e; e = e->next_callee)
     {
@@ -3398,8 +3451,6 @@ cgraph_node::verify_node (void)
       if (this_cfun->cfg)
 	{
 	  hash_set<gimple *> stmts;
-	  int i;
-	  ipa_ref *ref = NULL;
 
 	  /* Reach the trees by walking over the CFG, and note the
 	     enclosing basic-blocks in the call edges.  */
@@ -3468,6 +3519,13 @@ cgraph_node::verify_node (void)
 	/* No CFG available?!  */
 	gcc_unreachable ();
 
+      for (i = 0; iterate_reference (i, ref); i++)
+	if (ref->stmt && ref->lto_stmt_uid)
+	  {
+	    error ("reference has both cal_stmt and lto_stmt_uid set");
+	    error_found = true;
+	  }
+
       for (e = callees; e; e = e->next_callee)
 	{
 	  if (!e->aux && !e->speculative)
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index e73e0696b91..417488bca1f 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -132,7 +132,8 @@ cgraph_edge::clone (cgraph_node *n, gcall *call_stmt, unsigned stmt_uid,
 
   new_edge->inline_failed = inline_failed;
   new_edge->indirect_inlining_edge = indirect_inlining_edge;
-  new_edge->lto_stmt_uid = stmt_uid;
+  if (!call_stmt)
+    new_edge->lto_stmt_uid = stmt_uid;
   new_edge->speculative_id = speculative_id;
   /* Clone flags that depend on call_stmt availability manually.  */
   new_edge->can_throw_external = can_throw_external;
diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index 621607499e1..b0c7ebf775b 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -257,10 +257,11 @@ lto_output_edge (struct lto_simple_output_block *ob, struct cgraph_edge *edge,
   edge->count.stream_out (ob->main_stream);
 
   bp = bitpack_create (ob->main_stream);
-  uid = (!gimple_has_body_p (edge->caller->decl) || edge->caller->thunk.thunk_p
-	 ? edge->lto_stmt_uid : gimple_uid (edge->call_stmt) + 1);
+  uid = !edge->call_stmt ? edge->lto_stmt_uid
+			 : gimple_uid (edge->call_stmt) + 1;
   bp_pack_enum (&bp, cgraph_inline_failed_t,
 	        CIF_N_REASONS, edge->inline_failed);
+  gcc_checking_assert (uid || edge->caller->thunk.thunk_p);
   bp_pack_var_len_unsigned (&bp, uid);
   bp_pack_value (&bp, edge->speculative_id, 16);
   bp_pack_value (&bp, edge->indirect_inlining_edge, 1);
@@ -669,7 +670,7 @@ lto_output_ref (struct lto_simple_output_block *ob, struct ipa_ref *ref,
 {
   struct bitpack_d bp;
   int nref;
-  int uid = ref->lto_stmt_uid;
+  int uid = !ref->stmt ? ref->lto_stmt_uid : gimple_uid (ref->stmt) + 1;
   struct cgraph_node *node;
 
   bp = bitpack_create (ob->main_stream);
diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c
index 3e64371094e..9566e5ee102 100644
--- a/gcc/lto-streamer-in.c
+++ b/gcc/lto-streamer-in.c
@@ -892,6 +892,7 @@ fixup_call_stmt_edges_1 (struct cgraph_node *node, gimple **stmts,
         fatal_error (input_location,
 		     "Cgraph edge statement index out of range");
       cedge->call_stmt = as_a <gcall *> (stmts[cedge->lto_stmt_uid - 1]);
+      cedge->lto_stmt_uid = 0;
       if (!cedge->call_stmt)
         fatal_error (input_location,
 		     "Cgraph edge statement index not found");
@@ -902,6 +903,7 @@ fixup_call_stmt_edges_1 (struct cgraph_node *node, gimple **stmts,
         fatal_error (input_location,
 		     "Cgraph edge statement index out of range");
       cedge->call_stmt = as_a <gcall *> (stmts[cedge->lto_stmt_uid - 1]);
+      cedge->lto_stmt_uid = 0;
       if (!cedge->call_stmt)
         fatal_error (input_location, "Cgraph edge statement index not found");
     }
@@ -912,6 +914,7 @@ fixup_call_stmt_edges_1 (struct cgraph_node *node, gimple **stmts,
 	  fatal_error (input_location,
 		       "Reference statement index out of range");
 	ref->stmt = stmts[ref->lto_stmt_uid - 1];
+	ref->lto_stmt_uid = 0;
 	if (!ref->stmt)
 	  fatal_error (input_location, "Reference statement index not found");
       }

Patch

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 95b523d6be5..5fe2178e334 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1226,8 +1226,8 @@  cgraph_edge::resolve_speculation (cgraph_edge *edge, tree callee_decl)
         fprintf (dump_file, "Speculative call turned into direct call.\n");
       edge = e2;
       e2 = tmp;
-      /* FIXME:  If EDGE is inlined, we should scale up the frequencies and counts
-         in the functions inlined through it.  */
+      /* FIXME:  If EDGE is inlined, we should scale up the frequencies
+	 and counts in the functions inlined through it.  */
     }
   edge->count += e2->count;
   if (edge->num_speculative_call_targets_p ())
@@ -1263,11 +1263,52 @@  cgraph_edge::make_direct (cgraph_edge *edge, cgraph_node *callee)
   /* If we are redirecting speculative call, make it non-speculative.  */
   if (edge->speculative)
     {
-      edge = resolve_speculation (edge, callee->decl);
+      cgraph_edge *found = NULL;
+      cgraph_edge *direct, *next;
+      ipa_ref *ref;
+
+      edge->speculative_call_info (direct, edge, ref);
 
-      /* On successful speculation just return the pre existing direct edge.  */
-      if (!edge->indirect_unknown_callee)
-        return edge;
+      /* Look all speculative targets and remove all but one corresponding
+	 to callee (if it exists).
+	 If there is only one target we can save one extra call to
+	 speculative_call_info.  */
+      if (edge->num_speculative_call_targets_p () != 1)
+	for (direct = edge->caller->callees; direct; direct = next)
+	  {
+	    next = direct->next_callee;
+	    if (direct->call_stmt == edge->call_stmt
+		&& direct->lto_stmt_uid == edge->lto_stmt_uid)
+	      {
+		direct->speculative_call_info (direct, edge, ref);
+
+		/* Compare ref not direct->callee.  Direct edge is possibly
+		   inlined or redirected.  */
+		if (!ref->referred->semantically_equivalent_p (callee))
+		  edge = direct->resolve_speculation (direct, NULL);
+		else
+		  {
+		    gcc_checking_assert (!found);
+		    found = direct;
+		  }
+	      }
+	  }
+	else if (!ref->referred->semantically_equivalent_p (callee))
+	  edge = direct->resolve_speculation (direct, NULL);
+	else
+	  found = direct;
+
+      /* On successful speculation just remove the indirect edge and
+	 return the pre existing direct edge.
+	 It is important to not remove it and redirect because the direct
+	 edge may be inlined or redirected.  */
+      if (found)
+	{
+	  resolve_speculation (edge, callee->decl);
+	  gcc_checking_assert (!found->speculative);
+	  return found;
+	}
+      gcc_checking_assert (edge->speculative);
     }
 
   edge->indirect_unknown_callee = 0;
@@ -1328,7 +1369,7 @@  cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e)
       /* If there already is an direct call (i.e. as a result of inliner's
 	 substitution), forget about speculating.  */
       if (decl)
-	e = resolve_speculation (e, decl);
+	e = make_direct (e, cgraph_node::get (decl));
       else
 	{
 	  /* Expand speculation into GIMPLE code.  */
@@ -3116,6 +3157,8 @@  cgraph_node::verify_node (void)
   basic_block this_block;
   gimple_stmt_iterator gsi;
   bool error_found = false;
+  int i;
+  ipa_ref *ref = NULL;
 
   if (seen_error ())
     return;
@@ -3201,6 +3244,11 @@  cgraph_node::verify_node (void)
 	  cgraph_debug_gimple_stmt (this_cfun, e->call_stmt);
 	  error_found = true;
 	}
+      if (e->call_stmt && e->lto_stmt_uid)
+	{
+	  error ("edge has both cal_stmt and lto_stmt_uid set");
+	  error_found = true;
+	}
     }
   bool check_comdat = comdat_local_p ();
   for (e = callers; e; e = e->next_caller)
@@ -3267,6 +3315,11 @@  cgraph_node::verify_node (void)
 	  fprintf (stderr, "\n");
 	  error_found = true;
 	}
+      if (e->call_stmt && e->lto_stmt_uid)
+	{
+	  error ("edge has both cal_stmt and lto_stmt_uid set");
+	  error_found = true;
+	}
     }
   for (e = indirect_calls; e; e = e->next_callee)
     {
@@ -3398,8 +3451,6 @@  cgraph_node::verify_node (void)
       if (this_cfun->cfg)
 	{
 	  hash_set<gimple *> stmts;
-	  int i;
-	  ipa_ref *ref = NULL;
 
 	  /* Reach the trees by walking over the CFG, and note the
 	     enclosing basic-blocks in the call edges.  */
@@ -3468,6 +3519,13 @@  cgraph_node::verify_node (void)
 	/* No CFG available?!  */
 	gcc_unreachable ();
 
+      for (i = 0; iterate_reference (i, ref); i++)
+	if (ref->stmt && ref->lto_stmt_uid)
+	  {
+	    error ("reference has both cal_stmt and lto_stmt_uid set");
+	    error_found = true;
+	  }
+
       for (e = callees; e; e = e->next_callee)
 	{
 	  if (!e->aux && !e->speculative)
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index e73e0696b91..417488bca1f 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -132,7 +132,8 @@  cgraph_edge::clone (cgraph_node *n, gcall *call_stmt, unsigned stmt_uid,
 
   new_edge->inline_failed = inline_failed;
   new_edge->indirect_inlining_edge = indirect_inlining_edge;
-  new_edge->lto_stmt_uid = stmt_uid;
+  if (!call_stmt)
+    new_edge->lto_stmt_uid = stmt_uid;
   new_edge->speculative_id = speculative_id;
   /* Clone flags that depend on call_stmt availability manually.  */
   new_edge->can_throw_external = can_throw_external;
diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index 621607499e1..670c967f896 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -257,14 +257,21 @@  lto_output_edge (struct lto_simple_output_block *ob, struct cgraph_edge *edge,
   edge->count.stream_out (ob->main_stream);
 
   bp = bitpack_create (ob->main_stream);
-  uid = (!gimple_has_body_p (edge->caller->decl) || edge->caller->thunk.thunk_p
-	 ? edge->lto_stmt_uid : gimple_uid (edge->call_stmt) + 1);
+  uid = !edge->call_stmt ? edge->lto_stmt_uid
+			 : gimple_uid (edge->call_stmt) + 1;
   bp_pack_enum (&bp, cgraph_inline_failed_t,
 	        CIF_N_REASONS, edge->inline_failed);
+  gcc_checking_assert (uid);
   bp_pack_var_len_unsigned (&bp, uid);
   bp_pack_value (&bp, edge->speculative_id, 16);
   bp_pack_value (&bp, edge->indirect_inlining_edge, 1);
   bp_pack_value (&bp, edge->speculative, 1);
   bp_pack_value (&bp, edge->call_stmt_cannot_inline_p, 1);
   gcc_assert (!edge->call_stmt_cannot_inline_p
 	      || edge->inline_failed != CIF_BODY_NOT_AVAILABLE);
@@ -669,7 +676,7 @@  lto_output_ref (struct lto_simple_output_block *ob, struct ipa_ref *ref,
 {
   struct bitpack_d bp;
   int nref;
-  int uid = ref->lto_stmt_uid;
+  int uid = !ref->stmt ? ref->lto_stmt_uid : gimple_uid (ref->stmt) + 1;
   struct cgraph_node *node;
 
   bp = bitpack_create (ob->main_stream);
diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c
index 3e64371094e..9566e5ee102 100644
--- a/gcc/lto-streamer-in.c
+++ b/gcc/lto-streamer-in.c
@@ -892,6 +892,7 @@  fixup_call_stmt_edges_1 (struct cgraph_node *node, gimple **stmts,
         fatal_error (input_location,
 		     "Cgraph edge statement index out of range");
       cedge->call_stmt = as_a <gcall *> (stmts[cedge->lto_stmt_uid - 1]);
+      cedge->lto_stmt_uid = 0;
       if (!cedge->call_stmt)
         fatal_error (input_location,
 		     "Cgraph edge statement index not found");
@@ -902,6 +903,7 @@  fixup_call_stmt_edges_1 (struct cgraph_node *node, gimple **stmts,
         fatal_error (input_location,
 		     "Cgraph edge statement index out of range");
       cedge->call_stmt = as_a <gcall *> (stmts[cedge->lto_stmt_uid - 1]);
+      cedge->lto_stmt_uid = 0;
       if (!cedge->call_stmt)
         fatal_error (input_location, "Cgraph edge statement index not found");
     }
@@ -912,6 +914,7 @@  fixup_call_stmt_edges_1 (struct cgraph_node *node, gimple **stmts,
 	  fatal_error (input_location,
 		       "Reference statement index out of range");
 	ref->stmt = stmts[ref->lto_stmt_uid - 1];
+	ref->lto_stmt_uid = 0;
 	if (!ref->stmt)
 	  fatal_error (input_location, "Reference statement index not found");
       }