From 56cb815ba22dd2ec00fee7a38f0862bc21d1c2a9 Mon Sep 17 00:00:00 2001 From: Jan Hubicka Date: Tue, 13 Oct 2020 09:19:54 +0200 Subject: [PATCH] Fix tramp3d PGO misoptimization this patch fixes tramp3d ICE with PGO. It has turned out to be by a misupdate in ignore_edge I introduced in previous patch that made us to not compute SCCs correctly with -fno-lto. While looking for problem I proofread the sources and also fortified the srouces for situation where we insert a summary for no good reason and noticed a problem that early ipa-modref disabled itself in some cases. I also noticed that param_index is treamed as uhwi while it is signed (that wastes file space). Bootstrapping/regtesting x86_64-linux, will commit it tomorrow if that passes. gcc/ChangeLog: 2020-10-13 Jan Hubicka PR ipa/97389 * ipa-modref.c (dump_lto_records): Fix formating of dump file. (modref_summary::dump): Do not check loads to be non-null. (modref_summary_lto::dump): Do not check loads to be non-null. (merge_call_side_effects): Improve debug output. (analyze_call): Crash when cur_summary->loads is NULL. (analyze_function): Update. (modref_summaries::insert): Insert only into summaries, not optimization_summaries. (modref_summaries::duplicate): Likewise; crash when load or sotres are NULL. (modref_summaries_lto::duplicate): Crash when loads or stores are NULL. (write_modref_records): param_index is signed. (read_modref_records): param_index is signed. (modref_write): Crash when loads or stores are NULL. (read_section): Compensate previous change. (pass_modref::execute): Do not check optimization_summaries t be non-NULL. (ignore_edge): Fix. (compute_parm_map): Fix formating. (modref_propagate_in_scc): Do not expect loads/stores to be NULL. --- gcc/ipa-modref.c | 188 +++++++++++++++++++++-------------------------- 1 file changed, 84 insertions(+), 104 deletions(-) diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c index 1d4eaf8d7ad..4f86b9ccea1 100644 --- a/gcc/ipa-modref.c +++ b/gcc/ipa-modref.c @@ -298,7 +298,7 @@ dump_lto_records (modref_records_lto *tt, FILE *out) r->ref ? get_alias_set (r->ref) : 0); if (r->every_access) { - fprintf (out, " Every access\n"); + fprintf (out, " Every access\n"); continue; } size_t k; @@ -314,16 +314,10 @@ dump_lto_records (modref_records_lto *tt, FILE *out) void modref_summary::dump (FILE *out) { - if (loads) - { - fprintf (out, " loads:\n"); - dump_records (loads, out); - } - if (stores) - { - fprintf (out, " stores:\n"); - dump_records (stores, out); - } + fprintf (out, " loads:\n"); + dump_records (loads, out); + fprintf (out, " stores:\n"); + dump_records (stores, out); } /* Dump summary. */ @@ -331,16 +325,10 @@ modref_summary::dump (FILE *out) void modref_summary_lto::dump (FILE *out) { - if (loads) - { - fprintf (out, " loads:\n"); - dump_lto_records (loads, out); - } - if (stores) - { - fprintf (out, " stores:\n"); - dump_lto_records (stores, out); - } + fprintf (out, " loads:\n"); + dump_lto_records (loads, out); + fprintf (out, " stores:\n"); + dump_lto_records (stores, out); } /* Get function summary for FUNC if it exists, return NULL otherwise. */ @@ -530,16 +518,19 @@ ignore_stores_p (tree caller, int flags) bool merge_call_side_effects (modref_summary *cur_summary, gimple *stmt, modref_summary *callee_summary, - bool ignore_stores) + bool ignore_stores, cgraph_node *callee_node) { auto_vec parm_map; bool changed = false; + if (dump_file) + fprintf (dump_file, " - Merging side effects of %s with parm map:", + callee_node->dump_name ()); + parm_map.safe_grow_cleared (gimple_call_num_args (stmt)); for (unsigned i = 0; i < gimple_call_num_args (stmt); i++) { tree op = gimple_call_arg (stmt, i); - STRIP_NOPS (op); if (TREE_CODE (op) == SSA_NAME && SSA_NAME_IS_DEFAULT_DEF (op) && TREE_CODE (SSA_NAME_VAR (op)) == PARM_DECL) @@ -563,17 +554,17 @@ merge_call_side_effects (modref_summary *cur_summary, parm_map[i].parm_index = -2; else parm_map[i].parm_index = -1; + if (dump_file) + fprintf (dump_file, " %i", parm_map[i].parm_index); } + if (dump_file) + fprintf (dump_file, "\n"); /* Merge with callee's summary. */ - if (cur_summary->loads) - changed |= cur_summary->loads->merge (callee_summary->loads, &parm_map); + changed |= cur_summary->loads->merge (callee_summary->loads, &parm_map); if (!ignore_stores) - { - if (cur_summary->stores) - changed |= cur_summary->stores->merge (callee_summary->stores, - &parm_map); - } + changed |= cur_summary->stores->merge (callee_summary->stores, + &parm_map); return changed; } @@ -672,8 +663,7 @@ analyze_call (modref_summary *cur_summary, { if (ignore_stores) { - if (cur_summary->loads) - cur_summary->loads->collapse (); + cur_summary->loads->collapse (); return true; } if (dump_file) @@ -681,7 +671,8 @@ analyze_call (modref_summary *cur_summary, return false; } - merge_call_side_effects (cur_summary, stmt, callee_summary, ignore_stores); + merge_call_side_effects (cur_summary, stmt, callee_summary, ignore_stores, + callee_node); return true; } @@ -853,7 +844,10 @@ analyze_function (function *f, bool ipa) else /* Remove existing summary if we are re-running the pass. */ { if (dump_file - && optimization_summaries->get (cgraph_node::get (f->decl))) + && (summary + = optimization_summaries->get (cgraph_node::get (f->decl))) + != NULL + && summary->loads) { fprintf (dump_file, "Past summary:\n"); optimization_summaries->get @@ -950,7 +944,8 @@ analyze_function (function *f, bool ipa) (summary, recursive_calls[i], summary, ignore_stores_p (current_function_decl, gimple_call_flags - (recursive_calls[i]))); + (recursive_calls[i])), + fnode); if (!summary->useful_p (ecf_flags)) { remove_summary (lto, nolto, ipa); @@ -1005,18 +1000,19 @@ modref_generate (void) void modref_summaries::insert (struct cgraph_node *node, modref_summary *) { - if (!DECL_STRUCT_FUNCTION (node->decl)) + /* Local passes ought to be executed by the pass manager. */ + if (this == optimization_summaries) { optimization_summaries->remove (node); + return; + } + if (!DECL_STRUCT_FUNCTION (node->decl)) + { summaries->remove (node); + return; } push_cfun (DECL_STRUCT_FUNCTION (node->decl)); - /* This is not very pretty: We do not know if we insert into optimization - summary or summary. Do both but check for duplicated effort. */ - if (optimization_summaries && !optimization_summaries->get (node)->loads) - analyze_function (DECL_STRUCT_FUNCTION (node->decl), false); - if (summaries && !summaries->get (node)->loads) - analyze_function (DECL_STRUCT_FUNCTION (node->decl), true); + analyze_function (DECL_STRUCT_FUNCTION (node->decl), true); pop_cfun (); } @@ -1042,26 +1038,27 @@ modref_summaries_lto::insert (struct cgraph_node *node, modref_summary_lto *) /* Called when new clone is inserted to callgraph late. */ void -modref_summaries::duplicate (cgraph_node *, cgraph_node *, +modref_summaries::duplicate (cgraph_node *, cgraph_node *dst, modref_summary *src_data, modref_summary *dst_data) { - if (src_data->stores) + /* Do not duplicte optimization summaries; we do not handle parameter + transforms on them. */ + if (this == optimization_summaries) { - dst_data->stores = modref_records::create_ggc - (src_data->stores->max_bases, - src_data->stores->max_refs, - src_data->stores->max_accesses); - dst_data->stores->copy_from (src_data->stores); - } - if (src_data->loads) - { - dst_data->loads = modref_records::create_ggc - (src_data->loads->max_bases, - src_data->loads->max_refs, - src_data->loads->max_accesses); - dst_data->loads->copy_from (src_data->loads); + optimization_summaries->remove (dst); + return; } + dst_data->stores = modref_records::create_ggc + (src_data->stores->max_bases, + src_data->stores->max_refs, + src_data->stores->max_accesses); + dst_data->stores->copy_from (src_data->stores); + dst_data->loads = modref_records::create_ggc + (src_data->loads->max_bases, + src_data->loads->max_refs, + src_data->loads->max_accesses); + dst_data->loads->copy_from (src_data->loads); } /* Called when new clone is inserted to callgraph late. */ @@ -1071,22 +1068,16 @@ modref_summaries_lto::duplicate (cgraph_node *, cgraph_node *, modref_summary_lto *src_data, modref_summary_lto *dst_data) { - if (src_data->stores) - { - dst_data->stores = modref_records_lto::create_ggc - (src_data->stores->max_bases, - src_data->stores->max_refs, - src_data->stores->max_accesses); - dst_data->stores->copy_from (src_data->stores); - } - if (src_data->loads) - { - dst_data->loads = modref_records_lto::create_ggc - (src_data->loads->max_bases, - src_data->loads->max_refs, - src_data->loads->max_accesses); - dst_data->loads->copy_from (src_data->loads); - } + dst_data->stores = modref_records_lto::create_ggc + (src_data->stores->max_bases, + src_data->stores->max_refs, + src_data->stores->max_accesses); + dst_data->stores->copy_from (src_data->stores); + dst_data->loads = modref_records_lto::create_ggc + (src_data->loads->max_bases, + src_data->loads->max_refs, + src_data->loads->max_accesses); + dst_data->loads->copy_from (src_data->loads); } namespace @@ -1154,7 +1145,7 @@ write_modref_records (modref_records_lto *tt, struct output_block *ob) modref_access_node *access_node; FOR_EACH_VEC_SAFE_ELT (ref_node->accesses, k, access_node) { - streamer_write_uhwi (ob, access_node->parm_index); + streamer_write_hwi (ob, access_node->parm_index); if (access_node->parm_index != -1) { streamer_write_uhwi (ob, access_node->parm_offset_known); @@ -1278,7 +1269,7 @@ read_modref_records (lto_input_block *ib, struct data_in *data_in, for (size_t k = 0; k < naccesses; k++) { - int parm_index = streamer_read_uhwi (ib); + int parm_index = streamer_read_hwi (ib); bool parm_offset_known = false; poly_int64 parm_offset = 0; poly_int64 offset = 0; @@ -1358,12 +1349,8 @@ modref_write () streamer_write_uhwi (ob, lto_symtab_encoder_encode (encoder, cnode)); - streamer_write_uhwi (ob, r->loads ? 1 : 0); - streamer_write_uhwi (ob, r->stores ? 1 : 0); - if (r->loads) - write_modref_records (r->loads, ob); - if (r->stores) - write_modref_records (r->stores, ob); + write_modref_records (r->loads, ob); + write_modref_records (r->stores, ob); } } streamer_write_char_stream (ob->main_stream, 0); @@ -1406,8 +1393,6 @@ read_section (struct lto_file_decl_data *file_data, const char *data, modref_summary_lto *modref_sum_lto = summaries_lto ? summaries_lto->get_create (node) : NULL; - int have_loads = streamer_read_uhwi (&ib); - int have_stores = streamer_read_uhwi (&ib); if (optimization_summaries) modref_sum = optimization_summaries->get_create (node); @@ -1416,14 +1401,12 @@ read_section (struct lto_file_decl_data *file_data, const char *data, && !modref_sum->stores)); gcc_assert (!modref_sum_lto || (!modref_sum_lto->loads && !modref_sum_lto->stores)); - if (have_loads) - read_modref_records (&ib, data_in, - modref_sum ? &modref_sum->loads : NULL, - modref_sum_lto ? &modref_sum_lto->loads : NULL); - if (have_stores) - read_modref_records (&ib, data_in, - modref_sum ? &modref_sum->stores : NULL, - modref_sum_lto ? &modref_sum_lto->stores : NULL); + read_modref_records (&ib, data_in, + modref_sum ? &modref_sum->loads : NULL, + modref_sum_lto ? &modref_sum_lto->loads : NULL); + read_modref_records (&ib, data_in, + modref_sum ? &modref_sum->stores : NULL, + modref_sum_lto ? &modref_sum_lto->stores : NULL); if (dump_file) { fprintf (dump_file, "Read modref for %s\n", @@ -1598,9 +1581,6 @@ public: unsigned int pass_modref::execute (function *f) { - /* If new function is being added during IPA, we can skip analysis. */ - if (!optimization_summaries) - return 0; analyze_function (f, false); return 0; } @@ -1628,7 +1608,7 @@ ignore_edge (struct cgraph_edge *e) (&avail, e->caller); return (avail <= AVAIL_INTERPOSABLE - || ((!summaries || !summaries->get (callee)) + || ((!optimization_summaries || !optimization_summaries->get (callee)) && (!summaries_lto || !summaries_lto->get (callee))) || flags_from_decl_or_type (e->callee->decl) & (ECF_CONST | ECF_NOVOPS)); @@ -1684,7 +1664,7 @@ compute_parm_map (cgraph_edge *callee_edge, vec *parm_map) if (jf && jf->type == IPA_JF_PASS_THROUGH) { (*parm_map)[i].parm_index - = ipa_get_jf_pass_through_formal_id (jf); + = ipa_get_jf_pass_through_formal_id (jf); (*parm_map)[i].parm_offset_known = ipa_get_jf_pass_through_operation (jf) == NOP_EXPR; (*parm_map)[i].parm_offset = 0; @@ -1921,6 +1901,8 @@ modref_propagate_in_scc (cgraph_node *component_node) optimization_summaries->remove (node); if (summaries_lto) summaries_lto->remove (node); + cur_summary = NULL; + cur_summary_lto = NULL; changed = true; break; } @@ -2009,19 +1991,17 @@ modref_propagate_in_scc (cgraph_node *component_node) /* Merge in callee's information. */ if (callee_summary) { - if (callee_summary->loads) - changed |= cur_summary->loads->merge - (callee_summary->loads, &parm_map); - if (callee_summary->stores) + changed |= cur_summary->loads->merge + (callee_summary->loads, &parm_map); + if (!ignore_stores) changed |= cur_summary->stores->merge (callee_summary->stores, &parm_map); } if (callee_summary_lto) { - if (callee_summary_lto->loads) - changed |= cur_summary_lto->loads->merge - (callee_summary_lto->loads, &parm_map); - if (callee_summary_lto->stores) + changed |= cur_summary_lto->loads->merge + (callee_summary_lto->loads, &parm_map); + if (!ignore_stores) changed |= cur_summary_lto->stores->merge (callee_summary_lto->stores, &parm_map); } -- 2.30.2