Re: [google][gcc-4_8][patch]Handle Split functions in the Function Reordering Plugin

[Teresa Johnson <tejohnson at google dot com>]

On Tue, Feb 11, 2014 at 4:32 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, Feb 11, 2014 at 3:29 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>
>> On Feb 11, 2014 2:37 PM, "Sriraman Tallam" <tmsriram@google.com> wrote:
>>>
>>> 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
>>> >> +         spliandection.  */
>>
>>> >> +      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.
>>
>> The comment is fixed but the checks stayed the same and seem out of order
>> now. Teresa
>
> Ah!, sorry. Changed and patch attached.

The assert in the else clause is unnecessary (since you have landed
there after doing that same check already). It would be good to have
asserts in both the if and else clauses on the new section_name (see
my suggested code in the initial response).

Teresa

>
> Sri
>
>
>>
>>>
>>> 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



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413