On Tue, Feb 11, 2014 at 4:51 PM, Sriraman Tallam <tmsri...@google.com> wrote:
> On Tue, Feb 11, 2014 at 4:39 PM, Teresa Johnson <tejohn...@google.com> wrote:
>> On Tue, Feb 11, 2014 at 4:32 PM, Sriraman Tallam <tmsri...@google.com> wrote:
>>> On Tue, Feb 11, 2014 at 3:29 PM, Teresa Johnson <tejohn...@google.com>
>>> wrote:
>>>>
>>>> On Feb 11, 2014 2:37 PM, "Sriraman Tallam" <tmsri...@google.com> wrote:
>>>>>
>>>>> On Tue, Feb 11, 2014 at 1:32 PM, Teresa Johnson <tejohn...@google.com>
>>>>> wrote:
>>>>> > On Mon, Feb 10, 2014 at 7:15 PM, Sriraman Tallam <tmsri...@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).
>
> Ok, I overlooked the code you suggested in the original response,
> sorry about that. I have included those asserts you suggested in both
> places where we swap the new section with the existing section.
Ok, thanks! Looks good.
Teresa
>
> Thanks
> Sri
>
>>
>> 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 | tejohn...@google.com | 408-460-2413
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
