[google,gcc-4_8] Handle Split functions in the Function Reordering Plugin

On Tue, Feb 11, 2014 at 1:32 PM, Teresa Johnson <tejohnson@google.com> wrote:
> On Mon, Feb 10, 2014 at 7:15 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> Hi Teresa,
>>
>>    I have attached a patch to recognize and reorder split functions in
>> the function reordering plugin. Please review.
>>
>> Thanks
>> Sri
>
>> Index: function_reordering_plugin/callgraph.c
>> ===================================================================
>> --- function_reordering_plugin/callgraph.c    (revision 207671)
>> +++ function_reordering_plugin/callgraph.c    (working copy)
>> @@ -550,6 +550,25 @@ static void set_node_type (Node *n)
>>        s->computed_weight = n->computed_weight;
>>        s->max_count = n->max_count;
>>
>> +      /* If s is split into a cold section, assign the split weight to the
>> +         max count of the split section.   Use this also for the weight of the
>> +         split section.  */
>> +      if (s->split_section)
>> +        {
>> +          s->split_section->max_count = s->split_section->weight = n->split_weight;
>> +          /* If split_section is comdat, update all the comdat
>> +          candidates for weight.  */
>> +          s_comdat = s->split_section->comdat_group;
>> +          while (s_comdat != NULL)
>> +            {
>> +              s_comdat->weight = s->split_section->weight;
>> +              s_comdat->computed_weight
>> +             = s->split_section->computed_weight;
>
> Where is s->split_section->computed_weight set

I removed this line as it is not useful. Details:

In general, the computed_weight for sections is assigned in
set_node_type in line:
 s->computed_weight = n->computed_weight;

The computed_weight is obtained by adding the weights of all incoming
edges. However, for the cold part of split functions, this can never
be non-zero as the split cold part is never called and so this line is
not useful.

>
>> +              s_comdat->max_count = s->split_section->max_count;
>> +              s_comdat = s_comdat->comdat_group;
>> +            }
>> +     }
>> +
>
> ...
>
>
>> +      /* It is split and it is not comdat.  */
>> +      else if (is_split_function)
>> +     {
>> +       Section_id *cold_section = NULL;
>> +       /* Function splitting means that the "hot" part is really the
>> +          relevant section and the other section is unlikely executed and
>> +          should not be part of the callgraph.  */
>>
>> -      section->comdat_group = kept->comdat_group;
>> -      kept->comdat_group = section;
>> +       /* Store the new section in the section list.  */
>> +       section->next = first_section;
>> +       first_section = section;
>> +       /* The right section is not in the section_map if ".text.unlikely" is
>> +          not the new section.  */
>> +          if (!is_prefix_of (".text.unlikely", section_name))
>
> The double-negative in the above comment is a bit confusing. Can we
> make this similar to the check in the earlier split comdat case. I.e.
> something like (essentially swapping the if condition and assert):

Changed. New patch attached.

Thanks
Sri

>
>           /* If the existing section is cold, the newly detected split must
>              be hot.  */
>           if (is_prefix_of (".text.unlikely", kept->full_name))
>             {
>               assert (!is_prefix_of (".text.unlikely", section_name));
>               ...
>             }
>           else
>             {
>               assert (is_prefix_of (".text.unlikely", section_name));
>               ...
>             }
>
>> +         {
>> +           assert (is_prefix_of (".text.unlikely", kept->full_name));
>> +           /* The kept section was the unlikely section.  Change the section
>> +              in section_map to be the new section which is the hot one.  */
>> +           *slot = section;
>> +           /* Record the split cold section in the hot section.  */
>> +           section->split_section = kept;
>> +           /* Comdats and function splitting are already handled.  */
>> +           assert (kept->comdat_group == NULL);
>> +           cold_section = kept;
>> +         }
>> +       else
>> +         {
>> +           /* Record the split cold section in the hot section.  */
>> +           assert (!is_prefix_of (".text.unlikely", kept->full_name));
>> +           kept->split_section = section;
>> +           cold_section = section;
>> +         }
>> +       assert (cold_section != NULL && cold_section->comdat_group == NULL);
>> +       cold_section->is_split_cold_section = 1;
>> +     }
> ...
>
> Thanks,
> Teresa
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

[google,gcc-4_8] Handle Split functions in the Function Reordering Plugin

Commit Message

Patch