Re: [google] Linker plugin to do function reordering using callgraph edge profiles (issue5124041)

[Sriraman Tallam <tmsriram at google dot com>]

*Resending as plain text*

I am attaching a patch of the updated files. This patch was meant for
the google gcc-4_6 branch. Sorry for not mentioning this upfront in my
original mail.
Thanks,
 -Sri.



> On Mon, Sep 26, 2011 at 9:53 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>
>> On Mon, Sep 26, 2011 at 9:22 AM, Nathan Froyd <nfroyd@mozilla.com> wrote:
>> > On 9/23/2011 6:03 PM, Sriraman Tallam wrote:
>> >>
>> >> This patch adds a new linker plugin to re-order functions.
>> >
>> > This is great stuff. ?We were experimenting with using the coverage files to
>> > generate an ordering for --section-ordering-file, but this might be even
>> > better, will have to experiment with it.
>> >
>> > A couple of comments on the code itself:
>> >
>> >> Index: function_reordering_plugin/callgraph.h
>> >> +inline static Edge_list *
>> >> +make_edge_list (Edge *e)
>> >> +{
>> >> + ?Edge_list *list = (Edge_list *)malloc (sizeof (Edge_list));
>> >
>> > If you are going to use libiberty via hashtab.h, you might as well make use
>> > of the *ALLOC family of macros to make this and other allocations a little
>> > neater.
>> >
>> >> +/*Represents an edge in the call graph. ?*/
>> >> +struct __edge__
>> >
>> > I think the usual convention is to call this edge_d or something similar,
>> > avoiding the profusion of underscores.
>> >
>> >> +void
>> >> +map_section_name_to_index (char *section_name, void *handle, int shndx)
>> >> +{
>> >> + ?void **slot;
>> >> + ?char *function_name;
>> >> +
>> >> + ?if (is_prefix_of (".text.hot.", section_name))
>> >> + ? ?function_name = section_name + 10 /* strlen (".text.hot.") */;
>> >> + ?else if (is_prefix_of (".text.unlikely.", section_name))
>> >> + ? ?function_name = section_name + 15 /* strlen (".text.unlikely.") */;
>> >> + ?else if (is_prefix_of (".text.cold.", section_name))
>> >> + ? ?function_name = section_name + 11 /* strlen (".text.cold.") */;
>> >> + ?else if (is_prefix_of (".text.startup.", section_name))
>> >> + ? ?function_name = section_name + 14 /* strlen (".text.startup.") */;
>> >> + ?else
>> >> + ? ?function_name = section_name + 6 /*strlen (".text.") */;
>> >
>> > You don't handle plain .text; this is assuming -ffunction-sections, right?
>> > ?Can this limitation be easily removed?
>>
>> You need to compile with -ffunction-sections or the individual
>> sections cannot be re-ordered. That is why I assumed a .text.* prefix
>> before every function section. Did you have something else in mind?
>>
>> Thanks for your other comments. I will make those changes.
>>
>> -Sri.
>>
>> >
>> > I think it is customary to put the comment after the semicolon.
>> >
>> > Might just be easier to say something like:
>> >
>> > ?const char *sections[] = { ".text.hot", ... }
>> > ?char *function_name = NULL;
>> >
>> > ?for (i = 0; i < ARRAY_SIZE (sections); i++)
>> > ? ?if (is_prefix_of (sections[i], section_name))
>> > ? ? ?{
>> > ? ? ? ? function_name = section_name + strlen (sections[i]);
>> > ? ? ? ? break;
>> > ? ? ?}
>> >
>> > ?if (!function_name)
>> > ? ?function_name = section_name + 6; /* strlen (".text.") */
>> >
>> > I guess that's not much shorter.
>> >
>> >> +void
>> >> +cleanup ()
>> >> +{
>> >> + ?Node *node;
>> >> + ?Edge *edge;
>> >> +
>> >> + ?/* Free all nodes and edge_lists. ?*/
>> >> + ?for (node = node_chain; node != NULL; )
>> >> + ? ?{
>> >> + ? ? ?Node *next_node = node->next;
>> >> + ? ? ?Edge_list *it;
>> >> + ? ? ?for (it = node->edge_list; it != NULL; )
>> >> + ? ? ? ?{
>> >> + ? ? ? ? ?Edge_list *next_it = it->next;
>> >> + ? ? ? ? ?free (it);
>> >> + ? ? ? ? ?it = next_it;
>> >> + ? ? ? ?}
>> >
>> > Write a free_edge_list function, since you do this once here and twice
>> > below.
>> >
>> > -Nathan
>> >
>

Attachment: patch_new.txt
Description: Text document