> Hi, > while looking into firefox profiles, I noticed that we miss devirtualizations > to comdat symbols, because we manage to get different profile_id in each > unit. This is easily fixed by the following patch that makes profiled_id > to by crc32 of the symbol name in this case. > > Bootstrapped/regtested x86_64-linux, tested with firefox, will > commit it tomorrow.
Hi, this is version I comitted with minor change of using the coverage checksum because I think some of anonymous namespace functions do get exported with random seed attached in a very side case. To answer Martin's question, I am just removing the tp_first_run merging code becuase it is done twice - second time in ipa_merge_profiles. Jakub/Richi: I would like to see this in 4.9 (it is missed optimization compared to 4.8). Let me know when it is OK to commit it (perhaps minus the lto-symtab.c change that is not necessary). * ipa-utils.c (ipa_merge_profiles): Merge profile_id. * coverage.c (coverage_compute_profile_id): Handle externally visible symbols. * lto/lto-symtab.c (lto_cgraph_replace_node): Don't re-merge tp_first_run. Index: ipa-utils.c =================================================================== --- ipa-utils.c (revision 209385) +++ ipa-utils.c (working copy) @@ -660,6 +660,14 @@ ipa_merge_profiles (struct cgraph_node * if (dst->tp_first_run > src->tp_first_run && src->tp_first_run) dst->tp_first_run = src->tp_first_run; + if (src->profile_id) + { + if (!dst->profile_id) + dst->profile_id = src->profile_id; + else + gcc_assert (src->profile_id == dst->profile_id); + } + if (!dst->count) return; if (cgraph_dump_file) Index: coverage.c =================================================================== --- coverage.c (revision 209385) +++ coverage.c (working copy) @@ -555,18 +555,29 @@ coverage_compute_lineno_checksum (void) unsigned coverage_compute_profile_id (struct cgraph_node *n) { - expanded_location xloc - = expand_location (DECL_SOURCE_LOCATION (n->decl)); - unsigned chksum = xloc.line; + unsigned chksum; - chksum = coverage_checksum_string (chksum, xloc.file); - chksum = coverage_checksum_string - (chksum, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (n->decl))); - if (first_global_object_name) - chksum = coverage_checksum_string - (chksum, first_global_object_name); - chksum = coverage_checksum_string - (chksum, aux_base_name); + /* Externally visible symbols have unique name. */ + if (TREE_PUBLIC (n->decl) || DECL_EXTERNAL (n->decl)) + { + chksum = coverage_checksum_string + (0, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (n->decl))); + } + else + { + expanded_location xloc + = expand_location (DECL_SOURCE_LOCATION (n->decl)); + + chksum = xloc.line; + chksum = coverage_checksum_string (chksum, xloc.file); + chksum = coverage_checksum_string + (chksum, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (n->decl))); + if (first_global_object_name) + chksum = coverage_checksum_string + (chksum, first_global_object_name); + chksum = coverage_checksum_string + (chksum, aux_base_name); + } /* Non-negative integers are hopefully small enough to fit in all targets. */ return chksum & 0x7fffffff; Index: lto/lto-symtab.c =================================================================== --- lto/lto-symtab.c (revision 209385) +++ lto/lto-symtab.c (working copy) @@ -91,12 +91,6 @@ lto_cgraph_replace_node (struct cgraph_n if (node->decl != prevailing_node->decl) cgraph_release_function_body (node); - /* Time profile merging */ - if (node->tp_first_run) - prevailing_node->tp_first_run = prevailing_node->tp_first_run ? - MIN (prevailing_node->tp_first_run, node->tp_first_run) : - node->tp_first_run; - /* Finally remove the replaced node. */ cgraph_remove_node (node); }