Patchwork [GOOGLE] Refactor the LIPO fixup

login
register
mail settings
Submitter Dehao Chen
Date March 27, 2014, 4:02 p.m.
Message ID <CAO2gOZV1rzPHHBoL8FoyMsdTc7AuaZjMnzPpXr5xAshNL6EnXw@mail.gmail.com>
Download mbox | patch
Permalink /patch/334389/
State New
Headers show

Comments

Dehao Chen - March 27, 2014, 4:02 p.m.
On Wed, Mar 26, 2014 at 4:05 PM, Xinliang David Li <davidxl@google.com> wrote:
> is cgraph_init_gid_map called after linking?

Oh, forgot that part. It's interesting that the test can pass without
another cgraph_init_gid_map call.

Patch updated. Retested and the performance is OK.

Dehao

>
> David
>
> On Wed, Mar 26, 2014 at 3:54 PM, Dehao Chen <dehao@google.com> wrote:
>> Patch updated, passed performance tests.
>>
>> Dehao
>>
>> On Tue, Mar 25, 2014 at 4:03 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> Add comment to the new function. init_node_map is better invoked after
>>> the link step to avoid creating entries with for dead nodes.
>>>
>>> Ok if large perf testing is fine.
>>>
>>> David
>>>
>>> On Tue, Mar 25, 2014 at 3:38 PM, Dehao Chen <dehao@google.com> wrote:
>>>> This patch refactors LIPO fixup related code to move it into a
>>>> standalone function. This makes sure that
>>>> symtab_remove_unreachable_nodes is called right after the fixup so
>>>> that there is not dangling cgraph nodes any time.
>>>>
>>>> Bootstrapped and regression test on-going.
>>>>
>>>> OK for google-4_8?
>>>>
>>>> Thanks,
>>>> Dehao
Xinliang David Li - March 27, 2014, 6:14 p.m.
ok.

On Thu, Mar 27, 2014 at 9:02 AM, Dehao Chen <dehao@google.com> wrote:
> On Wed, Mar 26, 2014 at 4:05 PM, Xinliang David Li <davidxl@google.com> wrote:
>> is cgraph_init_gid_map called after linking?
>
> Oh, forgot that part. It's interesting that the test can pass without
> another cgraph_init_gid_map call.
>
> Patch updated. Retested and the performance is OK.
>
> Dehao
>
>>
>> David
>>
>> On Wed, Mar 26, 2014 at 3:54 PM, Dehao Chen <dehao@google.com> wrote:
>>> Patch updated, passed performance tests.
>>>
>>> Dehao
>>>
>>> On Tue, Mar 25, 2014 at 4:03 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> Add comment to the new function. init_node_map is better invoked after
>>>> the link step to avoid creating entries with for dead nodes.
>>>>
>>>> Ok if large perf testing is fine.
>>>>
>>>> David
>>>>
>>>> On Tue, Mar 25, 2014 at 3:38 PM, Dehao Chen <dehao@google.com> wrote:
>>>>> This patch refactors LIPO fixup related code to move it into a
>>>>> standalone function. This makes sure that
>>>>> symtab_remove_unreachable_nodes is called right after the fixup so
>>>>> that there is not dangling cgraph nodes any time.
>>>>>
>>>>> Bootstrapped and regression test on-going.
>>>>>
>>>>> OK for google-4_8?
>>>>>
>>>>> Thanks,
>>>>> Dehao

Patch

Index: gcc/cgraphbuild.c
===================================================================
--- gcc/cgraphbuild.c	(revision 208869)
+++ gcc/cgraphbuild.c	(working copy)
@@ -244,9 +244,6 @@  add_fake_indirect_call_edges (struct cgraph_node *
   if (!L_IPO_COMP_MODE)
     return;
 
-  if (cgraph_pre_profiling_inlining_done)
-    return;
-
   ic_counts
       = get_coverage_counts_no_warn (DECL_STRUCT_FUNCTION (node->symbol.decl),
                                      GCOV_COUNTER_ICALL_TOPNV, &n_counts);
@@ -599,7 +596,7 @@  record_references_in_initializer (tree decl, bool
    needs to be set to the resolved node so that ipa-inline
    sees the definitions.  */
 #include "gimple-pretty-print.h"
-void
+static void
 lipo_fixup_cgraph_edge_call_target (gimple stmt)
 {
   tree decl;
@@ -625,6 +622,58 @@  lipo_fixup_cgraph_edge_call_target (gimple stmt)
     }
 }
 
+/* Link the cgraph nodes, varpool nodes and fixup the call target to
+   the correct decl. Remove dead functions.  */
+
+
+void
+lipo_link_and_fixup ()
+{
+  struct cgraph_node *node;
+
+  cgraph_pre_profiling_inlining_done = true;
+  cgraph_process_module_scope_statics ();
+  /* Now perform link to allow cross module inlining.  */
+  cgraph_do_link ();
+  varpool_do_link ();
+  cgraph_unify_type_alias_sets ();
+  cgraph_init_gid_map ();
+ 
+  FOR_EACH_DEFINED_FUNCTION (node)
+    {
+      if (!gimple_has_body_p (node->symbol.decl))
+	continue;
+
+      /* Don't profile functions produced for builtin stuff.  */
+      if (DECL_SOURCE_LOCATION (node->symbol.decl) == BUILTINS_LOCATION)
+	continue;
+
+      push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl));
+
+      if (L_IPO_COMP_MODE)
+	{
+	  basic_block bb;
+	  FOR_EACH_BB (bb)
+	    {
+	      gimple_stmt_iterator gsi;
+	      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+		{
+		  gimple stmt = gsi_stmt (gsi);
+		  if (is_gimple_call (stmt))
+		    lipo_fixup_cgraph_edge_call_target (stmt);
+		}
+	    }
+	  update_ssa (TODO_update_ssa);
+	}
+      rebuild_cgraph_edges ();
+      pop_cfun ();
+    }
+
+  cgraph_add_fake_indirect_call_edges ();
+  symtab_remove_unreachable_nodes (true, dump_file);
+}
+
+
 /* Rebuild cgraph edges for current function node.  This needs to be run after
    passes that don't update the cgraph.  */
 
@@ -677,7 +726,8 @@  rebuild_cgraph_edges (void)
 				       mark_load, mark_store, mark_address);
     }
 
-  add_fake_indirect_call_edges (node);
+  if (!cgraph_pre_profiling_inlining_done)
+    add_fake_indirect_call_edges (node);
   record_eh_tables (node, cfun);
   gcc_assert (!node->global.inlined_to);
 
Index: gcc/l-ipo.h
===================================================================
--- gcc/l-ipo.h	(revision 208869)
+++ gcc/l-ipo.h	(working copy)
@@ -60,7 +60,7 @@  void add_decl_to_current_module_scope (tree decl,
 int lipo_cmp_type (tree t1, tree t2);
 tree get_type_or_decl_name (tree);
 int equivalent_struct_types_for_tbaa (const_tree t1, const_tree t2);
-void lipo_fixup_cgraph_edge_call_target (gimple);
+void lipo_link_and_fixup (void);
 extern void copy_defined_module_set (tree, tree);
 extern bool is_parsing_done_p (void);
 extern const char* get_module_name (unsigned int);
Index: gcc/tree-profile.c
===================================================================
--- gcc/tree-profile.c	(revision 208869)
+++ gcc/tree-profile.c	(working copy)
@@ -1118,19 +1118,12 @@  tree_profiling (void)
   /* This is a small-ipa pass that gets called only once, from
      cgraphunit.c:ipa_passes().  */
   gcc_assert (cgraph_state == CGRAPH_STATE_IPA_SSA);
-
   /* After value profile transformation, artificial edges (that keep
      function body from being deleted) won't be needed.  */
+  if (L_IPO_COMP_MODE)
+    lipo_link_and_fixup ();
+  init_node_map ();
 
-  cgraph_pre_profiling_inlining_done = true;
-  cgraph_process_module_scope_statics ();
-  /* Now perform link to allow cross module inlining.  */
-  cgraph_do_link ();
-  varpool_do_link ();
-  cgraph_unify_type_alias_sets ();
-
-  init_node_map();
-
   FOR_EACH_DEFINED_FUNCTION (node)
     {
       if (!gimple_has_body_p (node->symbol.decl))
@@ -1142,23 +1135,6 @@  tree_profiling (void)
 
       push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl));
 
-      if (L_IPO_COMP_MODE)
-        {
-          basic_block bb;
-          FOR_EACH_BB (bb)
-            {
-              gimple_stmt_iterator gsi;
-              for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
-                {
-                  gimple stmt = gsi_stmt (gsi);
-                  if (is_gimple_call (stmt))
-                    lipo_fixup_cgraph_edge_call_target (stmt);
-                }
-	    }
-          update_ssa (TODO_update_ssa);
-	}
-
-
       /* Local pure-const may imply need to fixup the cfg.  */
       if (execute_fixup_cfg () & TODO_cleanup_cfg)
 	cleanup_tree_cfg ();
Index: gcc/value-prof.c
===================================================================
--- gcc/value-prof.c	(revision 208869)
+++ gcc/value-prof.c	(working copy)
@@ -1255,11 +1255,9 @@  init_gid_map (void)
 {
   struct cgraph_node *n;
 
-  gcc_assert (!gid_map);
+  if (!gid_map)
+    gid_map = htab_create (10, htab_gid_hash, htab_gid_eq, htab_gid_del);
 
-  gid_map
-      = htab_create (10, htab_gid_hash, htab_gid_eq, htab_gid_del);
-
   FOR_EACH_FUNCTION (n)
     {
       func_gid_entry_t ent, *entp;
@@ -1286,6 +1284,15 @@  init_gid_map (void)
           entp->node = n;
           entp->gid = ent.gid;
         }
+      else if (cgraph_pre_profiling_inlining_done)
+	{
+	  (*slot)->node = cgraph_lipo_get_resolved_node (n->symbol.decl);
+	  (*slot)->gid = ent.gid;
+	}
+      else
+	{
+	  gcc_assert ((*slot)->gid == ent.gid && (*slot)->node == n);
+	}
     }
 }
 
Index: gcc/auto-profile.c
===================================================================
--- gcc/auto-profile.c	(revision 208869)
+++ gcc/auto-profile.c	(working copy)
@@ -1533,16 +1533,11 @@  auto_profile (void)
   if (cgraph_state == CGRAPH_STATE_FINISHED)
     return 0;
 
-  init_node_map ();
   profile_info = autofdo::afdo_profile_info;
+  if (L_IPO_COMP_MODE)
+    lipo_link_and_fixup ();
+  init_node_map ();
 
-  cgraph_pre_profiling_inlining_done = true;
-  cgraph_process_module_scope_statics ();
-  /* Now perform link to allow cross module inlining.  */
-  cgraph_do_link ();
-  varpool_do_link ();
-  cgraph_unify_type_alias_sets ();
-
   FOR_EACH_FUNCTION (node)
     {
       if (!gimple_has_body_p (node->symbol.decl))
@@ -1554,35 +1549,6 @@  auto_profile (void)
 
       push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl));
 
-      if (L_IPO_COMP_MODE)
-	{
-	  basic_block bb;
-	  FOR_EACH_BB (bb)
-	    {
-	      gimple_stmt_iterator gsi;
-	      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
-		{
-		  gimple stmt = gsi_stmt (gsi);
-		  if (is_gimple_call (stmt))
-		    lipo_fixup_cgraph_edge_call_target (stmt);
-		}
-	    }
-	}
-      rebuild_cgraph_edges ();
-      pop_cfun ();
-    }
-
-  FOR_EACH_FUNCTION (node)
-    {
-      if (!gimple_has_body_p (node->symbol.decl))
-	continue;
-
-      /* Don't profile functions produced for builtin stuff.  */
-      if (DECL_SOURCE_LOCATION (node->symbol.decl) == BUILTINS_LOCATION)
-	continue;
-
-      push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl));
-
       /* First do indirect call promotion and early inline to make the
 	 IR match the profiled binary before actual annotation.