| Submitter | Sriraman Tallam |
|---|---|
| Date | Jan. 4, 2013, 1:41 a.m. |
| Message ID | <CAAs8HmxS+hKy7CWdhtbp93cRSo6aF5NdVzgOep=yoiYqF2exOA@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/209345/ |
| State | New |
| Headers | show |
Comments
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. >>> >>>
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. >>>> >>>>
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) @@ -34,8 +34,12 @@ 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. */ + stderr to --plugin-opt. + This plugin also allows placing all functions found cold in a separate + segment. This can be enabled with the linker option: + --plugin-opt,segment=cold. */ + #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 unlikely_segment + is set. This can be set with the linker option: + "--plugin-opt,segment=cold". */ +static int unlikely_segment = 0; /* Copies new output file name out_file */ void get_filename (const char *name) @@ -132,10 +137,11 @@ process_option (const char *name) /* Check if options is "segment=none" */ 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, "none") == 0) + unlikely_segment = 0; + else if (strcmp (option_val, "cold") == 0) + unlikely_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 (unlikely_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 (unlikely_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; }