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

[Nathan Froyd]

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?

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