diff mbox

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

Message ID CAAs8HmwJ2ta+sGz3i3gmbWg5DdmZ3r5yGnx=W5oSrmGSsc90Rg@mail.gmail.com
State New
Headers show

Commit Message

Sriraman Tallam Feb. 12, 2014, 12:32 a.m. UTC 
On Tue, Feb 11, 2014 at 3:29 PM, Teresa Johnson <tejohnson@google.com> wrote:
>
> On Feb 11, 2014 2:37 PM, "Sriraman Tallam" <tmsriram@google.com> wrote:
>>
>> On Tue, Feb 11, 2014 at 1:32 PM, Teresa Johnson <tejohnson@google.com>
>> wrote:
>> > 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
>> >> +         spliandection.  */
>
>> >> +      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
>>
>> I removed this line as it is not useful. Details:
>>
>> In general, the computed_weight for sections is assigned in
>> set_node_type in line:
>>  s->computed_weight = n->computed_weight;
>>
>> The computed_weight is obtained by adding the weights of all incoming
>> edges. However, for the cold part of split functions, this can never
>> be non-zero as the split cold part is never called and so this line is
>> not useful.
>>
>>
>> >
>> >> +              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):
>>
>> Changed. New patch attached.
>
> The comment is fixed but the checks stayed the same and seem out of order
> now. Teresa

Ah!, sorry. Changed and patch attached.

Sri


>
>>
>> Thanks
>> Sri
>>
>> >
>> >           /* 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
>> >
>> >
>> > --
>> > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

Comments

Teresa Johnson Feb. 12, 2014, 12:39 a.m. UTC | #1 
On Tue, Feb 11, 2014 at 4:32 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, Feb 11, 2014 at 3:29 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>
>> On Feb 11, 2014 2:37 PM, "Sriraman Tallam" <tmsriram@google.com> wrote:
>>>
>>> On Tue, Feb 11, 2014 at 1:32 PM, Teresa Johnson <tejohnson@google.com>
>>> wrote:
>>> > 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
>>> >> +         spliandection.  */
>>
>>> >> +      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
>>>
>>> I removed this line as it is not useful. Details:
>>>
>>> In general, the computed_weight for sections is assigned in
>>> set_node_type in line:
>>>  s->computed_weight = n->computed_weight;
>>>
>>> The computed_weight is obtained by adding the weights of all incoming
>>> edges. However, for the cold part of split functions, this can never
>>> be non-zero as the split cold part is never called and so this line is
>>> not useful.
>>>
>>>
>>> >
>>> >> +              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):
>>>
>>> Changed. New patch attached.
>>
>> The comment is fixed but the checks stayed the same and seem out of order
>> now. Teresa
>
> Ah!, sorry. Changed and patch attached.

The assert in the else clause is unnecessary (since you have landed
there after doing that same check already). It would be good to have
asserts in both the if and else clauses on the new section_name (see
my suggested code in the initial response).

Teresa

>
> Sri
>
>
>>
>>>
>>> Thanks
>>> Sri
>>>
>>> >
>>> >           /* 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
>>> >
>>> >
>>> > --
>>> > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
diff mbox

Patch

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" } } */
Index: gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_8.C
===================================================================
--- gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_8.C	(revision 207700)
+++ gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_8.C	(working copy)
@@ -15,5 +15,5 @@  int main()
 }
 
 /* { dg-final-use { scan-file-not linker.dump "Callgraph group" } }  */
-/* { dg-final-use { scan-file linker.dump "=== Unlikely sections start ===\n.*\.text\.hot\._Z3foov.* entry count = 1 computed = 1 max count = 1\n.*=== Unlikely sections end ===" } }  */
-/* { dg-final-use { remove-build-file "linker.dump" } }  */
+/* { dg-final-use { scan-file linker.dump "=== Unlikely sections start ===\n.*\.text\.hot\._Z3foov.* entry count = 1 computed = 1 max count = 1 split = 0\n.*=== Unlikely sections end ===" } }  */
+/*  dg-final-use { remove-build-file "linker.dump" } }  */
Index: function_reordering_plugin/callgraph.c
===================================================================
--- function_reordering_plugin/callgraph.c	(revision 207700)
+++ function_reordering_plugin/callgraph.c	(working copy)
@@ -550,6 +550,27 @@  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)
+            {
+	      /* Set the different weights for comdat candidates.  No need to se
+		 computed_weight as it is zero for split sections.  A split cold
+		 section is never called, it is only jumped into from the parent
+		 section.  */
+              s_comdat->weight = s->split_section->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 +720,128 @@  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;
+	  /* If the existing section is cold, the newly detected split must 
+	     be hot.  */
+          if (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 +1135,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: function_reordering_plugin/callgraph.h
===================================================================
--- function_reordering_plugin/callgraph.h	(revision 207700)
+++ 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;
 }