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
> + 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?
> + 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):
/* 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
