diff mbox

[google,4.7] Allow function reordering linker plugin to separate hot and cold code into different ELF segments

Message ID CAAs8HmwrSWdY=ZHYKVT2oe0-6n_-Zw3qXufXT6Ph-w2nEAm4DA@mail.gmail.com
State New
Headers show

Commit Message

Sriraman Tallam Jan. 4, 2013, 10:19 p.m. UTC
Attached new patch.

Thanks,
-Sri.

On Fri, Jan 4, 2013 at 9:12 AM, Rong Xu <xur@google.com> wrote:
> The code looks fine to me. Please consider David's comments about the
> option name.
>
> -Rong
>
> On Thu, Jan 3, 2013 at 9:14 PM, Xinliang David Li <davidxl@google.com> wrote:
>> Is it better to change the option to something like:
>>
>> split_segment|nosplit-segment
>> or split_segment=yes|no
>>
>>
>> David
>>
>> On Thu, Jan 3, 2013 at 5:41 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>> Hi Rong,
>>>
>>>   The following patch modifies the behaviour of the linker plugin to
>>> not create a separate segment for cold sections by default. Separate
>>> segments can be created with the plugin option "segment=cold". Is this
>>> alright to commit?
>>>
>>> Thanks,
>>> -Sri.
>>>
>>> On Mon, Dec 17, 2012 at 11:14 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>> I have committed this patch.
>>>>
>>>> Thanks,
>>>> -Sri.
>>>>
>>>> On Fri, Dec 14, 2012 at 4:16 PM, Rong Xu <xur@google.com> wrote:
>>>>> Looks good to me for google/gcc-4_7 branch.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Rong
>>>>>
>>>>>
>>>>> On Fri, Dec 14, 2012 at 3:42 PM, Sriraman Tallam <tmsriram@google.com>
>>>>> wrote:
>>>>>>
>>>>>> Hi Rong,
>>>>>>
>>>>>>     Please review this code. This code allows the function reordering
>>>>>> plugin to separate hot and cold code into different ELF segments.
>>>>>> This would allow optimizations like mapping the hot code alone to huge
>>>>>> pages.
>>>>>>
>>>>>>     With this patch, by default, the plugin maps .text.unlikely
>>>>>> sections into a separate ELF segment.  This can be turned off with
>>>>>> plugin option "--segment=none".
>>>>>>
>>>>>>     The include/plugin-api.h changes are a backport from trunk.
>>>>>>
>>>>>> Thanks,
>>>>>> -Sri.
>>>>>
>>>>>

Comments

Xinliang David Li Jan. 4, 2013, 10:32 p.m. UTC | #1
Looks good -- but better with followup :
1) give a warning when the parameter to the option is not allowed;
2) add test cases if possible.

David


On Fri, Jan 4, 2013 at 2:19 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> Attached new patch.
>
> Thanks,
> -Sri.
>
> On Fri, Jan 4, 2013 at 9:12 AM, Rong Xu <xur@google.com> wrote:
>> The code looks fine to me. Please consider David's comments about the
>> option name.
>>
>> -Rong
>>
>> On Thu, Jan 3, 2013 at 9:14 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> Is it better to change the option to something like:
>>>
>>> split_segment|nosplit-segment
>>> or split_segment=yes|no
>>>
>>>
>>> David
>>>
>>> On Thu, Jan 3, 2013 at 5:41 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>> Hi Rong,
>>>>
>>>>   The following patch modifies the behaviour of the linker plugin to
>>>> not create a separate segment for cold sections by default. Separate
>>>> segments can be created with the plugin option "segment=cold". Is this
>>>> alright to commit?
>>>>
>>>> Thanks,
>>>> -Sri.
>>>>
>>>> On Mon, Dec 17, 2012 at 11:14 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>> I have committed this patch.
>>>>>
>>>>> Thanks,
>>>>> -Sri.
>>>>>
>>>>> On Fri, Dec 14, 2012 at 4:16 PM, Rong Xu <xur@google.com> wrote:
>>>>>> Looks good to me for google/gcc-4_7 branch.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Rong
>>>>>>
>>>>>>
>>>>>> On Fri, Dec 14, 2012 at 3:42 PM, Sriraman Tallam <tmsriram@google.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Rong,
>>>>>>>
>>>>>>>     Please review this code. This code allows the function reordering
>>>>>>> plugin to separate hot and cold code into different ELF segments.
>>>>>>> This would allow optimizations like mapping the hot code alone to huge
>>>>>>> pages.
>>>>>>>
>>>>>>>     With this patch, by default, the plugin maps .text.unlikely
>>>>>>> sections into a separate ELF segment.  This can be turned off with
>>>>>>> plugin option "--segment=none".
>>>>>>>
>>>>>>>     The include/plugin-api.h changes are a backport from trunk.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> -Sri.
>>>>>>
>>>>>>
diff mbox

Patch

Index: function_reordering_plugin/function_reordering_plugin.c
===================================================================
--- function_reordering_plugin/function_reordering_plugin.c	(revision 194878)
+++ function_reordering_plugin/function_reordering_plugin.c	(working copy)
@@ -33,9 +33,13 @@  along with this program; see the file COPYING3.  I
 
    This plugin dumps the final layout order of the functions in a file
    called "final_layout.txt".  To change the output file, pass the new
-   file name with --plugin-opt.  To dump to stderr instead, just pass
-   stderr to --plugin-opt.  */
+   file name with --plugin-opt,file=<name>.  To dump to stderr instead,
+   just pass stderr as the file name.
 
+   This plugin also allows placing all functions found cold in a separate
+   segment.  This can be enabled with the linker option:
+   --plugin-opt,split_segment=yes.  */
+
 #if HAVE_STDINT_H
 #include <stdint.h>
 #endif
@@ -89,9 +93,10 @@  static int is_api_exist = 0;
 /* The plugin does nothing when no-op is 1.  */
 static int no_op = 0;
 
-/* The plugin does not create a new segment for unlikely code if
-   no_segment is set.  */
-static int no_segment = 0;
+/* The plugin creates a new segment for unlikely code if split_segment
+   is set.  This can be set with the linker option:
+   "--plugin-opt,split_segment=yes".  */
+static int split_segment = 0;
 
 /* Copies new output file name out_file  */
 void get_filename (const char *name)
@@ -110,7 +115,7 @@  process_option (const char *name)
 {
   const char *option_group = "group=";
   const char *option_file = "file=";
-  const char *option_segment = "segment=";
+  const char *option_segment = "split_segment=";
 
   /* Check if option is "group="  */
   if (strncmp (name, option_group, strlen (option_group)) == 0)
@@ -129,13 +134,14 @@  process_option (const char *name)
       return;
     }
 
-  /* Check if options is "segment=none"  */
+  /* Check if options is "split_segment=[yes|no]"  */
   if (strncmp (name, option_segment, strlen (option_segment)) == 0)
     {
-      if (strcmp (name + strlen (option_segment), "none") == 0)
-	no_segment = 1;
-      else
-	no_segment = 0;
+      const char *option_val = name + strlen (option_segment);
+      if (strcmp (option_val, "no") == 0)
+	split_segment = 0;
+      else if (strcmp (option_val, "yes") == 0)
+	split_segment = 1;
       return;
     }
 
@@ -244,7 +250,10 @@  claim_file_hook (const struct ld_plugin_input_file
     {
       /* Inform the linker to prepare for section reordering.  */
       (*allow_section_ordering) ();
-      (*allow_unique_segment_for_sections) ();
+      /* Inform the linker to allow certain sections to be placed in
+	 a separate segment.  */
+      if (split_segment == 1)
+        (*allow_unique_segment_for_sections) ();
       is_ordering_specified = 1;
     }
 
@@ -335,15 +344,29 @@  all_symbols_read_hook (void)
   if (out_file != NULL
       && strcmp (out_file, "stderr") != 0)
     fclose (fp);
-  /* Pass the new order of functions to the linker.  */
-  update_section_order (section_list, unlikely_segment_start);
-  assert (num_entries >= unlikely_segment_end);
-  update_section_order (section_list, num_entries - unlikely_segment_end);
-  /* Map all unlikely code into a new segment.  */
-  if (no_segment == 0)
-    unique_segment_for_sections (".text.unlikely_executed", 0, 0x1000,
-				 section_list + unlikely_segment_start,
-				 unlikely_segment_end - unlikely_segment_start);
+
+  if (split_segment == 1)
+    {
+      /* Pass the new order of functions to the linker.  */
+      /* Fix the order of all sections upto the beginning of the
+	 unlikely section.  */
+      update_section_order (section_list, unlikely_segment_start);
+      assert (num_entries >= unlikely_segment_end);
+      /* Fix the order of all sections after the end of the unlikely
+	 section.  */
+      update_section_order (section_list, num_entries - unlikely_segment_end);
+      /* Map all unlikely code into a new segment.  */
+      unique_segment_for_sections (
+	  ".text.unlikely_executed", 0, 0x1000,
+	  section_list + unlikely_segment_start,
+	  unlikely_segment_end - unlikely_segment_start);
+    }
+  else
+    {
+      /* Pass the new order of functions to the linker.  */
+      update_section_order (section_list, num_entries);
+    }
+
   cleanup ();
   return LDPS_OK;
 }