diff mbox

[google,gcc-4_8] Handle Split functions in the Function Reordering Plugin

Message ID CAAs8HmybQKqYjgYV1O5_tx0uzviqxu62W+bEXUO3xr9CS7cqvw@mail.gmail.com
State New
Headers show

Commit Message

Sriraman Tallam Feb. 11, 2014, 3:15 a.m. UTC
Hi Teresa,

   I have attached a patch to recognize and reorder split functions in
the function reordering plugin. Please review.

Thanks
Sri
This enhances the linker plugin to reorder functions that are split when
using -freorder-blocks-and-partition.

Comments

Teresa Johnson Feb. 11, 2014, 9:32 p.m. UTC | #1
On Mon, Feb 10, 2014 at 7:15 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> Hi Teresa,
>
>    I have attached a patch to recognize and reorder split functions in
> the function reordering plugin. Please review.
>
> Thanks
> Sri

> Index: function_reordering_plugin/callgraph.c
> ===================================================================
> --- function_reordering_plugin/callgraph.c    (revision 207671)
> +++ function_reordering_plugin/callgraph.c    (working copy)
> @@ -550,6 +550,25 @@ static void set_node_type (Node *n)
>        s->computed_weight = n->computed_weight;
>        s->max_count = n->max_count;
>
> +      /* If s is split into a cold section, assign the split weight to the
> +         max count of the split section.   Use this also for the weight of the
> +         split section.  */
> +      if (s->split_section)
> +        {
> +          s->split_section->max_count = s->split_section->weight = n->split_weight;
> +          /* If split_section is comdat, update all the comdat
> +          candidates for weight.  */
> +          s_comdat = s->split_section->comdat_group;
> +          while (s_comdat != NULL)
> +            {
> +              s_comdat->weight = s->split_section->weight;
> +              s_comdat->computed_weight
> +             = s->split_section->computed_weight;

Where is s->split_section->computed_weight set?

> +              s_comdat->max_count = s->split_section->max_count;
> +              s_comdat = s_comdat->comdat_group;
> +            }
> +     }
> +

...


> +      /* It is split and it is not comdat.  */
> +      else if (is_split_function)
> +     {
> +       Section_id *cold_section = NULL;
> +       /* Function splitting means that the "hot" part is really the
> +          relevant section and the other section is unlikely executed and
> +          should not be part of the callgraph.  */
>
> -      section->comdat_group = kept->comdat_group;
> -      kept->comdat_group = section;
> +       /* Store the new section in the section list.  */
> +       section->next = first_section;
> +       first_section = section;
> +       /* The right section is not in the section_map if ".text.unlikely" is
> +          not the new section.  */
> +          if (!is_prefix_of (".text.unlikely", section_name))

The double-negative in the above comment is a bit confusing. Can we
make this similar to the check in the earlier split comdat case. I.e.
something like (essentially swapping the if condition and assert):

          /* If the existing section is cold, the newly detected split must
             be hot.  */
          if (is_prefix_of (".text.unlikely", kept->full_name))
            {
              assert (!is_prefix_of (".text.unlikely", section_name));
              ...
            }
          else
            {
              assert (is_prefix_of (".text.unlikely", section_name));
              ...
            }

> +         {
> +           assert (is_prefix_of (".text.unlikely", kept->full_name));
> +           /* The kept section was the unlikely section.  Change the section
> +              in section_map to be the new section which is the hot one.  */
> +           *slot = section;
> +           /* Record the split cold section in the hot section.  */
> +           section->split_section = kept;
> +           /* Comdats and function splitting are already handled.  */
> +           assert (kept->comdat_group == NULL);
> +           cold_section = kept;
> +         }
> +       else
> +         {
> +           /* Record the split cold section in the hot section.  */
> +           assert (!is_prefix_of (".text.unlikely", kept->full_name));
> +           kept->split_section = section;
> +           cold_section = section;
> +         }
> +       assert (cold_section != NULL && cold_section->comdat_group == NULL);
> +       cold_section->is_split_cold_section = 1;
> +     }
...

Thanks,
Teresa
diff mbox

Patch

Index: function_reordering_plugin/callgraph.h
===================================================================
--- function_reordering_plugin/callgraph.h	(revision 207671)
+++ function_reordering_plugin/callgraph.h	(working copy)
@@ -236,6 +236,8 @@  typedef struct section_id_
      is comdat hot and kept, pointer to the kept cold split
      section.  */
   struct section_id_ *split_section;
+  /* If this is the cold part of a split section.  */
+  char is_split_cold_section;
   /* Check if this section has been considered for output.  */
   char processed;
 } Section_id;
@@ -260,6 +262,7 @@  make_section_id (char *name, char *full_name,
   s->computed_weight = 0;
   s->max_count = 0;
   s->split_section = NULL;
+  s->is_split_cold_section = 0;
 
   return s;
 }
Index: function_reordering_plugin/callgraph.c
===================================================================
--- function_reordering_plugin/callgraph.c	(revision 207671)
+++ function_reordering_plugin/callgraph.c	(working copy)
@@ -550,6 +550,25 @@  static void set_node_type (Node *n)
       s->computed_weight = n->computed_weight; 
       s->max_count = n->max_count;
 
+      /* If s is split into a cold section, assign the split weight to the
+         max count of the split section.   Use this also for the weight of the
+         split section.  */
+      if (s->split_section)
+        {
+          s->split_section->max_count = s->split_section->weight = n->split_weight;
+          /* If split_section is comdat, update all the comdat
+    	     candidates for weight.  */
+          s_comdat = s->split_section->comdat_group;
+          while (s_comdat != NULL)
+            {
+              s_comdat->weight = s->split_section->weight;
+              s_comdat->computed_weight
+		= s->split_section->computed_weight;
+              s_comdat->max_count = s->split_section->max_count;
+              s_comdat = s_comdat->comdat_group;
+            }
+	}
+
       /* If s is comdat, update all the comdat candidates for weight.  */
       s_comdat = s->comdat_group;
       while (s_comdat != NULL)
@@ -699,26 +718,129 @@  map_section_name_to_index (char *section_name, voi
     }
   else
     {
-      /* The function already exists, it must be a COMDAT.  Only one section
-	 in the comdat group will be kept, we don't know which.  Chain all the
-         comdat sections in the same comdat group to be emitted together later.
-         Keep one section as representative (kept) and update its section_type
-         to be equal to the type of the highest priority section in the
-         group.  */
+      /* Handle function splitting here.  With function splitting, the split
+         function sections have the same name and they are in the same module.
+	 Here, we only care about the section that is marked with prefix
+	 like ".text.hot".  The other section is cold.  The plugin should not
+	 be adding this cold section to the section_map.  In get_layout it will
+	 later be picked up when processing the non-callgraph sections and it
+	 will laid out appropriately.  */
       Section_id *kept = (Section_id *)(*slot);
       Section_id *section = make_section_id (function_name, section_name,
                                              section_type, handle, shndx);
+      int is_split_function = 0;
+      Section_id *split_comdat = NULL;
+      /* Check if this is a split function. The modules are the same means this
+	 is not comdat and we assume it is split.  It can be split and comdat
+	 too, in which case we have to search the comdat list of kept.  */
+      if (kept->handle == handle)
+	is_split_function = 1;
+      else if (kept->comdat_group != NULL)
+	{
+	  split_comdat = kept;
+	  do
+	    {
+	      if (split_comdat->comdat_group->handle == handle)
+		break;
+	      split_comdat = split_comdat->comdat_group;
+	    }
+	  while (split_comdat->comdat_group != NULL);
+	}
 
-      /* Two comdats in the same group can have different priorities.  This
-	 ensures that the "kept" comdat section has the priority of the higest
-  	 section in that comdat group.   This is necessary because the plugin
-	 does not know which section will be kept.  */
-      if (section_priority[kept->section_type]
-	  > section_priority[section_type])
-        kept->section_type = section_type;
+      /* It is split and it is comdat.  */
+      if (split_comdat != NULL
+	  && split_comdat->comdat_group != NULL)
+	{
+	  /* The comdat_section that is split.  */
+	  Section_id *comdat_section = split_comdat->comdat_group;
+	  Section_id *cold_section = NULL;
+	  /* If the existing section is cold, the newly detected split must 
+	     be hot.  */
+	  if (is_prefix_of (".text.unlikely", comdat_section->full_name))
+	    {
+	      cold_section = comdat_section;
+	      /* Replace the comdat_section in the kept section list with the
+		 new section.  */
+	      split_comdat->comdat_group = section;
+	      section->comdat_group = comdat_section->comdat_group;
+	      comdat_section->comdat_group = NULL;
+	    }
+	  else
+	    {
+	      cold_section = section;
+	    }
+	  assert (cold_section != NULL && cold_section->comdat_group == NULL);
+	  cold_section->is_split_cold_section = 1;
+	  /* The cold section must be added to the unlikely chain of comdat
+	     groups.  */
+	  if (kept->split_section == NULL)
+	    {	
+	      /* This happens if no comdat function in this group so far has
+		 been split.  */
+	      kept->split_section = cold_section;
+	    }
+	  else
+	    {
+	      /* Add the cold_section to the unlikely chain of comdats.  */
+	      cold_section->comdat_group = kept->split_section->comdat_group;
+	      kept->split_section->comdat_group = cold_section;
+	    }
+	}
+      /* It is split and it is not comdat.  */
+      else if (is_split_function)
+	{
+	  Section_id *cold_section = NULL;
+	  /* Function splitting means that the "hot" part is really the
+	     relevant section and the other section is unlikely executed and
+	     should not be part of the callgraph.  */
 
-      section->comdat_group = kept->comdat_group;
-      kept->comdat_group = section;
+	  /* Store the new section in the section list.  */
+	  section->next = first_section;
+	  first_section = section;
+	  /* The right section is not in the section_map if ".text.unlikely" is
+	     not the new section.  */
+          if (!is_prefix_of (".text.unlikely", section_name))
+	    {
+	      assert (is_prefix_of (".text.unlikely", kept->full_name));
+	      /* The kept section was the unlikely section.  Change the section
+		 in section_map to be the new section which is the hot one.  */
+	      *slot = section;
+	      /* Record the split cold section in the hot section.  */
+	      section->split_section = kept;
+	      /* Comdats and function splitting are already handled.  */
+	      assert (kept->comdat_group == NULL);
+	      cold_section = kept;
+	    }
+	  else
+	    {
+	      /* Record the split cold section in the hot section.  */
+	      assert (!is_prefix_of (".text.unlikely", kept->full_name));
+	      kept->split_section = section;
+	      cold_section = section;
+	    }
+	  assert (cold_section != NULL && cold_section->comdat_group == NULL);
+	  cold_section->is_split_cold_section = 1;
+	}
+      else
+	{
+          /* The function already exists, it must be a COMDAT.  Only one section
+	     in the comdat group will be kept, we don't know which.  Chain all
+	     the comdat sections in the same comdat group to be emitted
+	     together later.  Keep one section as representative (kept) and
+	     update its section_type to be equal to the type of the highest
+	     priority section in the group.  */
+
+          /* Two comdats in the same group can have different priorities.  This
+	     ensures that the "kept" comdat section has the priority of the
+	     highest section in that comdat group.   This is necessary because
+	     the plugin does not know which section will be kept.  */
+          if (section_priority[kept->section_type]
+	      > section_priority[section_type])
+            kept->section_type = section_type;
+
+          section->comdat_group = kept->comdat_group;
+          kept->comdat_group = section;
+	}
     }
 }
 
@@ -1012,8 +1134,10 @@  get_layout (FILE *fp, void*** handles,
 	  if (fp != NULL)
 	    {
 	      fprintf (fp, "%s entry count = %llu computed = %llu "
-		       "max count = %llu\n", s_it->full_name, s_it->weight,
-		       s_it->computed_weight, s_it->max_count);
+		       "max count = %llu split = %d\n", 
+		       s_it->full_name, s_it->weight,
+		       s_it->computed_weight, s_it->max_count,
+		       s_it->is_split_cold_section);
 	    }
 	  s_it = s_it->group;
         }
Index: gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_split_functions_1.C
===================================================================
--- gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_split_functions_1.C	(revision 0)
+++ gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_split_functions_1.C	(revision 0)
@@ -0,0 +1,58 @@ 
+/* Check if the gold function reordering plugin reorders split functions.
+   Check if foo is split and the cold section of foo is not next to its hot
+   section*/
+/* { dg-require-section-exclude "" } */
+/* { dg-require-linker-function-reordering-plugin "" } */
+/* { dg-require-effective-target freorder } */
+/* { dg-options "-O2 -freorder-functions=callgraph -ffunction-sections -freorder-blocks-and-partition --save-temps -Wl,-plugin-opt,file=linker.dump" } */
+
+
+#define SIZE 10000
+
+const char *sarr[SIZE];
+const char *buf_hot;
+const char *buf_cold;
+
+__attribute__ ((noinline))
+int bar (int *arg)
+{
+  (*arg)++;
+  return 0;
+}
+
+__attribute__((noinline))
+void 
+foo (int path)
+{
+  int i;
+  bar (&path);
+  if (path)
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_hot;
+    }
+  else
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_cold;
+    }
+}
+
+int
+main (int argc, char *argv[])
+{
+  buf_hot =  "hello";
+  buf_cold = "world";
+  foo (argc);
+  return 0;
+}
+
+/* { dg-final-use { scan-assembler "\.string \"ColdWeight 0\"" } } */
+/* { dg-final-use { scan-assembler "\.section.*\.text\.hot\._Z3fooi" } } */
+/* { dg-final-use { scan-assembler "\.section.*\.text\.unlikely\._Z3fooi" } } */
+/* { dg-final-use { cleanup-saved-temps } }  */
+/* { dg-final-use { scan-file linker.dump "Callgraph group : _Z3barPi _Z3fooi main\n" } }  */
+/* { dg-final-use { scan-file linker.dump "\.text\.unlikely\._Z3fooi .* split = 1" } } */
+/* { dg-final-use { scan-file linker.dump "\.text\.unlikely\._Z3fooi\[^\n\]*\n\.text\.unlikely\._Z3barPi\[^\n\]*\n" } }  */
+/* { dg-final-use { scan-file linker.dump "\.text\._Z3barPi\[^\n\]*\n\.text\.hot\._Z3fooi" } }  */
+/* { dg-final-use { remove-build-file "linker.dump" } } */