Message ID | 20200119104641.GB15937@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
Series | Fix two issues with multi-target speculative calls | expand |
> 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"); }
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"); }