diff mbox

Fix indirect call profiling for COMDAT symbols

Message ID 20140414172237.GA15009@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka April 14, 2014, 5:22 p.m. UTC 
> 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.

Comments

Jakub Jelinek April 14, 2014, 5:25 p.m. UTC | #1 
On Mon, Apr 14, 2014 at 07:22:37PM +0200, Jan Hubicka wrote:
> > 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.
> 
> 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).

For missed optimization at this point I'd prefer not to put it into 4.9.0
GA, for whether it is ok for 4.9.1 I'll defer to Richi.

	Jakub
Richard Biener April 15, 2014, 7:31 a.m. UTC | #2 
On Mon, 14 Apr 2014, Jakub Jelinek wrote:

> On Mon, Apr 14, 2014 at 07:22:37PM +0200, Jan Hubicka wrote:
> > > 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.
> > 
> > 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).
> 
> For missed optimization at this point I'd prefer not to put it into 4.9.0
> GA, for whether it is ok for 4.9.1 I'll defer to Richi.

If it doesn't cause issues on trunk it's ok for 4.9.1.

Richard.
diff mbox

Patch

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);
 }