diff mbox series

Fix profile merging for thunks

Message ID 20190103101801.njd7ja3hcgc4sa4m@kam.mff.cuni.cz
State New
Headers show
Series Fix profile merging for thunks | expand

Commit Message

Jan Hubicka Jan. 3, 2019, 10:18 a.m. UTC
Hi,
testing of Firefox has uncovered an ICE which I introduced by the patch
to fix profile of thunks.  The problem is that ipa_merge_profile may try
to merge profile of thunks and end up with attempting to touch their
gimple body.

This patch implements merging of profile of thunks and I also added
aliases because these also have no body and merging can happen.

Bootstrapped/regtested x86_64-linux, comitted.

	* ipa-utils.c (scale_ipa_profile_for_fn): Break out from ...
	(ipa_merge_profiles): ... here; do not ICE on thunks and aliases.

Comments

Jan Hubicka Jan. 3, 2019, 3:44 p.m. UTC | #1
Hi,
since Martin in meantime backported the fix for thunk profiling which
trigers the merging bug, I also bacported the patch into GCC 8
as follows.

Bootstrapped/regtested x86_64-linux and tested on Firefox build.

2019-01-03  Jan Hubicka  <hubicka@ucw.cz>

	* ipa-utils.c (scale_ipa_profile_for_fn): Break out from ...
	(ipa_merge_profiles): ... here; do not ICE on thunks and aliases.

Index: ipa-utils.c
===================================================================
--- ipa-utils.c	(revision 267549)
+++ ipa-utils.c	(working copy)
@@ -375,6 +375,20 @@ get_base_var (tree t)
   return t;
 }
 
+/* Scale function of calls in NODE by ratio ORIG_COUNT/NODE->count.  */
+
+void
+scale_ipa_profile_for_fn (struct cgraph_node *node, profile_count orig_count)
+{
+  profile_count to = node->count;
+  profile_count::adjust_for_ipa_scaling (&to, &orig_count);
+  struct cgraph_edge *e;
+  
+  for (e = node->callees; e; e = e->next_callee)
+    e->count = e->count.apply_scale (to, orig_count);
+  for (e = node->indirect_calls; e; e = e->next_callee)
+    e->count = e->count.apply_scale (to, orig_count);
+}
 
 /* SRC and DST are going to be merged.  Take SRC's profile and merge it into
    DST so it is not going to be lost.  Possibly destroy SRC's body on the way
@@ -392,6 +406,7 @@ ipa_merge_profiles (struct cgraph_node *
   if (!src->definition
       || !dst->definition)
     return;
+
   if (src->frequency < dst->frequency)
     src->frequency = dst->frequency;
 
@@ -402,6 +417,10 @@ ipa_merge_profiles (struct cgraph_node *
   if (src->profile_id && !dst->profile_id)
     dst->profile_id = src->profile_id;
 
+  /* Merging zero profile to dst is no-op.  */
+  if (src->count.ipa () == profile_count::zero ())
+    return;
+
   /* FIXME when we merge in unknown profile, we ought to set counts as
      unsafe.  */
   if (!src->count.initialized_p ()
@@ -412,11 +431,21 @@ ipa_merge_profiles (struct cgraph_node *
       fprintf (symtab->dump_file, "Merging profiles of %s to %s\n",
 	       src->dump_name (), dst->dump_name ());
     }
+  profile_count orig_count = dst->count;
+
   if (dst->count.initialized_p () && dst->count.ipa () == dst->count)
     dst->count += src->count.ipa ();
   else 
     dst->count = src->count.ipa ();
 
+  /* First handle functions with no gimple body.  */
+  if (dst->thunk.thunk_p || dst->alias
+      || src->thunk.thunk_p || src->alias)
+    {
+      scale_ipa_profile_for_fn (dst, orig_count);
+      return;
+    }
+
   /* This is ugly.  We need to get both function bodies into memory.
      If declaration is merged, we need to duplicate it to be able
      to load body that is being replaced.  This makes symbol table
@@ -641,7 +670,10 @@ ipa_merge_profiles (struct cgraph_node *
         src->release_body ();
       ipa_update_overall_fn_summary (dst);
     }
-  /* TODO: if there is no match, we can scale up.  */
+  /* We can't update CFG profile, but we can scale IPA profile. CFG
+     will be scaled according to dst->count after IPA passes.  */
+  else
+    scale_ipa_profile_for_fn (dst, orig_count);
   src->decl = oldsrcdecl;
 }
diff mbox series

Patch

Index: ipa-utils.c
===================================================================
--- ipa-utils.c	(revision 267499)
+++ ipa-utils.c	(working copy)
@@ -375,6 +375,20 @@  get_base_var (tree t)
   return t;
 }
 
+/* Scale function of calls in NODE by ratio ORIG_COUNT/NODE->count.  */
+
+void
+scale_ipa_profile_for_fn (struct cgraph_node *node, profile_count orig_count)
+{
+  profile_count to = node->count;
+  profile_count::adjust_for_ipa_scaling (&to, &orig_count);
+  struct cgraph_edge *e;
+  
+  for (e = node->callees; e; e = e->next_callee)
+    e->count = e->count.apply_scale (to, orig_count);
+  for (e = node->indirect_calls; e; e = e->next_callee)
+    e->count = e->count.apply_scale (to, orig_count);
+}
 
 /* SRC and DST are going to be merged.  Take SRC's profile and merge it into
    DST so it is not going to be lost.  Possibly destroy SRC's body on the way
@@ -424,6 +438,14 @@  ipa_merge_profiles (struct cgraph_node *
   else 
     dst->count = src->count.ipa ();
 
+  /* First handle functions with no gimple body.  */
+  if (dst->thunk.thunk_p || dst->alias
+      || src->thunk.thunk_p || src->alias)
+    {
+      scale_ipa_profile_for_fn (dst, orig_count);
+      return;
+    }
+
   /* This is ugly.  We need to get both function bodies into memory.
      If declaration is merged, we need to duplicate it to be able
      to load body that is being replaced.  This makes symbol table
@@ -652,16 +674,7 @@  ipa_merge_profiles (struct cgraph_node *
   /* We can't update CFG profile, but we can scale IPA profile. CFG
      will be scaled according to dst->count after IPA passes.  */
   else
-    {
-      profile_count to = dst->count;
-      profile_count::adjust_for_ipa_scaling (&to, &orig_count);
-      struct cgraph_edge *e;
-      
-      for (e = dst->callees; e; e = e->next_callee)
-	e->count = e->count.apply_scale (to, orig_count);
-      for (e = dst->indirect_calls; e; e = e->next_callee)
-	e->count = e->count.apply_scale (to, orig_count);
-    }
+    scale_ipa_profile_for_fn (dst, orig_count);
   src->decl = oldsrcdecl;
 }