On Fri, Jan 4, 2013 at 2:32 PM, Xinliang David Li <davi...@google.com> wrote: > 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.
Made all the changes. Modified the test case to check if the segment splitting API is invoked. The gold linker has a test case to check if the segment API actually splits segments. Thanks, -Sri. > > David > > > On Fri, Jan 4, 2013 at 2:19 PM, Sriraman Tallam <tmsri...@google.com> wrote: >> Attached new patch. >> >> Thanks, >> -Sri. >> >> On Fri, Jan 4, 2013 at 9:12 AM, Rong Xu <x...@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 <davi...@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 <tmsri...@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 <tmsri...@google.com> >>>>> wrote: >>>>>> I have committed this patch. >>>>>> >>>>>> Thanks, >>>>>> -Sri. >>>>>> >>>>>> On Fri, Dec 14, 2012 at 4:16 PM, Rong Xu <x...@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 <tmsri...@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. >>>>>>> >>>>>>>
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) @@ -121,22 +126,26 @@ process_option (const char *name) no_op = 0; return; } - /* Check if option is "file=" */ - if (strncmp (name, option_file, strlen (option_file)) == 0) + else if (strncmp (name, option_file, strlen (option_file)) == 0) { get_filename (name + strlen (option_file)); return; } - - /* Check if options is "segment=none" */ - if (strncmp (name, option_segment, strlen (option_segment)) == 0) + /* Check if options is "split_segment=[yes|no]" */ + else if (strncmp (name, option_segment, strlen (option_segment)) == 0) { - if (strcmp (name + strlen (option_segment), "none") == 0) - no_segment = 1; - else - no_segment = 0; - return; + const char *option_val = name + strlen (option_segment); + if (strcmp (option_val, "no") == 0) + { + split_segment = 0; + return; + } + else if (strcmp (option_val, "yes") == 0) + { + split_segment = 1; + return; + } } /* Unknown option, set no_op to 1. */ @@ -244,7 +253,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; } @@ -332,18 +344,35 @@ all_symbols_read_hook (void) section_list[i].shndx = shndx[i]; } + 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); + if (fp != NULL) + fprintf (fp, "Moving %u section(s) to new segment\n", + unlikely_segment_end - unlikely_segment_start); + } + else + { + /* Pass the new order of functions to the linker. */ + update_section_order (section_list, num_entries); + } + 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); + cleanup (); return LDPS_OK; } Index: gcc/testsuite/g++.dg/tree-prof/callgraph-profiles.C =================================================================== --- gcc/testsuite/g++.dg/tree-prof/callgraph-profiles.C (revision 194878) +++ gcc/testsuite/g++.dg/tree-prof/callgraph-profiles.C (working copy) @@ -2,7 +2,7 @@ with -freorder-functions=. */ /* { dg-require-section-exclude "" } */ /* { dg-require-linker-function-reordering-plugin "" } */ -/* { dg-options "-O2 -freorder-functions=callgraph -ffunction-sections --save-temps -Wl,-plugin-opt,file=callgraph-profiles.C.dump" } */ +/* { dg-options "-O2 -freorder-functions=callgraph -ffunction-sections --save-temps -Wl,-plugin-opt,file=callgraph-profiles.C.dump -Wl,-plugin-opt,split_segment=yes" } */ int notcalled () @@ -36,5 +36,6 @@ int main () /* { dg-final-use { scan-assembler "\.string \"1000\"" } } */ /* { dg-final-use { scan-file callgraph-profiles.C.dump "Callgraph group : main _Z3barv _Z3foov\n" } } */ /* { dg-final-use { scan-file callgraph-profiles.C.dump "\.text\.*\.main\n.text\.*\._Z3barv\n\.text\.*\._Z3foov\n\.text\.*\._Z9notcalledv" } } */ +/* { dg-final-use { scan-file callgraph-profiles.C.dump "Moving 1 section\\(s\\) to new segment" } } */ /* { dg-final-use { cleanup-saved-temps } } */ /* { dg-final-use { remove-build-file "callgraph-profiles.C.dump" } } */