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.
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
Index: gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_8.C
===================================================================
--- gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_8.C (revision
207700)
+++ gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_8.C (working copy)
@@ -15,5 +15,5 @@ int main()
}
/* { dg-final-use { scan-file-not linker.dump "Callgraph group" } } */
-/* { dg-final-use { scan-file linker.dump "=== Unlikely sections start
===\n.*\.text\.hot\._Z3foov.* entry count = 1 computed = 1 max count = 1\n.*===
Unlikely sections end ===" } } */
-/* { dg-final-use { remove-build-file "linker.dump" } } */
+/* { dg-final-use { scan-file linker.dump "=== Unlikely sections start
===\n.*\.text\.hot\._Z3foov.* entry count = 1 computed = 1 max count = 1 split
= 0\n.*=== Unlikely sections end ===" } } */
+/* dg-final-use { remove-build-file "linker.dump" } } */
Index:
gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_split_functions_1.C
===================================================================
--- gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_split_functions_1.C
(revision 0)
+++ gcc/testsuite/g++.dg/tree-prof/func_reorder_gold_plugin_split_functions_1.C
(revision 0)
@@ -0,0 +1,58 @@
+/* Check if the gold function reordering plugin reorders split functions.
+ Check if foo is split and the cold section of foo is not next to its hot
+ section*/
+/* { dg-require-section-exclude "" } */
+/* { dg-require-linker-function-reordering-plugin "" } */
+/* { dg-require-effective-target freorder } */
+/* { dg-options "-O2 -freorder-functions=callgraph -ffunction-sections
-freorder-blocks-and-partition --save-temps -Wl,-plugin-opt,file=linker.dump" }
*/
+
+
+#define SIZE 10000
+
+const char *sarr[SIZE];
+const char *buf_hot;
+const char *buf_cold;
+
+__attribute__ ((noinline))
+int bar (int *arg)
+{
+ (*arg)++;
+ return 0;
+}
+
+__attribute__((noinline))
+void
+foo (int path)
+{
+ int i;
+ bar (&path);
+ if (path)
+ {
+ for (i = 0; i < SIZE; i++)
+ sarr[i] = buf_hot;
+ }
+ else
+ {
+ for (i = 0; i < SIZE; i++)
+ sarr[i] = buf_cold;
+ }
+}
+
+int
+main (int argc, char *argv[])
+{
+ buf_hot = "hello";
+ buf_cold = "world";
+ foo (argc);
+ return 0;
+}
+
+/* { dg-final-use { scan-assembler "\.string \"ColdWeight 0\"" } } */
+/* { dg-final-use { scan-assembler "\.section.*\.text\.hot\._Z3fooi" } } */
+/* { dg-final-use { scan-assembler "\.section.*\.text\.unlikely\._Z3fooi" } }
*/
+/* { dg-final-use { cleanup-saved-temps } } */
+/* { dg-final-use { scan-file linker.dump "Callgraph group : _Z3barPi _Z3fooi
main\n" } } */
+/* { dg-final-use { scan-file linker.dump "\.text\.unlikely\._Z3fooi .* split
= 1" } } */
+/* { dg-final-use { scan-file linker.dump
"\.text\.unlikely\._Z3fooi\[^\n\]*\n\.text\.unlikely\._Z3barPi\[^\n\]*\n" } }
*/
+/* { dg-final-use { scan-file linker.dump
"\.text\._Z3barPi\[^\n\]*\n\.text\.hot\._Z3fooi" } } */
+/* { dg-final-use { remove-build-file "linker.dump" } } */
Index: function_reordering_plugin/callgraph.c
===================================================================
--- function_reordering_plugin/callgraph.c (revision 207700)
+++ function_reordering_plugin/callgraph.c (working copy)
@@ -550,6 +550,27 @@ 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)
+ {
+ /* Set the different weights for comdat candidates. No need to se
+ computed_weight as it is zero for split sections. A split cold
+ section is never called, it is only jumped into from the parent
+ section. */
+ s_comdat->weight = s->split_section->weight;
+ s_comdat->max_count = s->split_section->max_count;
+ s_comdat = s_comdat->comdat_group;
+ }
+ }
+
/* If s is comdat, update all the comdat candidates for weight. */
s_comdat = s->comdat_group;
while (s_comdat != NULL)
@@ -699,26 +720,131 @@ map_section_name_to_index (char *section_name, voi
}
else
{
- /* The function already exists, it must be a COMDAT. Only one section
- in the comdat group will be kept, we don't know which. Chain all the
- comdat sections in the same comdat group to be emitted together later.
- Keep one section as representative (kept) and update its section_type
- to be equal to the type of the highest priority section in the
- group. */
+ /* Handle function splitting here. With function splitting, the split
+ function sections have the same name and they are in the same module.
+ Here, we only care about the section that is marked with prefix
+ like ".text.hot". The other section is cold. The plugin should not
+ be adding this cold section to the section_map. In get_layout it will
+ later be picked up when processing the non-callgraph sections and it
+ will laid out appropriately. */
Section_id *kept = (Section_id *)(*slot);
Section_id *section = make_section_id (function_name, section_name,
section_type, handle, shndx);
+ int is_split_function = 0;
+ Section_id *split_comdat = NULL;
+ /* Check if this is a split function. The modules are the same means this
+ is not comdat and we assume it is split. It can be split and comdat
+ too, in which case we have to search the comdat list of kept. */
+ if (kept->handle == handle)
+ is_split_function = 1;
+ else if (kept->comdat_group != NULL)
+ {
+ split_comdat = kept;
+ do
+ {
+ if (split_comdat->comdat_group->handle == handle)
+ break;
+ split_comdat = split_comdat->comdat_group;
+ }
+ while (split_comdat->comdat_group != NULL);
+ }
- /* Two comdats in the same group can have different priorities. This
- ensures that the "kept" comdat section has the priority of the higest
- section in that comdat group. This is necessary because the plugin
- does not know which section will be kept. */
- if (section_priority[kept->section_type]
- > section_priority[section_type])
- kept->section_type = section_type;
+ /* It is split and it is comdat. */
+ if (split_comdat != NULL
+ && split_comdat->comdat_group != NULL)
+ {
+ /* The comdat_section that is split. */
+ Section_id *comdat_section = split_comdat->comdat_group;
+ Section_id *cold_section = NULL;
+ /* If the existing section is cold, the newly detected split must
+ be hot. */
+ if (is_prefix_of (".text.unlikely", comdat_section->full_name))
+ {
+ assert (!is_prefix_of (".text.unlikely", section_name));
+ cold_section = comdat_section;
+ /* Replace the comdat_section in the kept section list with the
+ new section. */
+ split_comdat->comdat_group = section;
+ section->comdat_group = comdat_section->comdat_group;
+ comdat_section->comdat_group = NULL;
+ }
+ else
+ {
+ assert (is_prefix_of (".text.unlikely", section_name));
+ cold_section = section;
+ }
+ assert (cold_section != NULL && cold_section->comdat_group == NULL);
+ cold_section->is_split_cold_section = 1;
+ /* The cold section must be added to the unlikely chain of comdat
+ groups. */
+ if (kept->split_section == NULL)
+ {
+ /* This happens if no comdat function in this group so far has
+ been split. */
+ kept->split_section = cold_section;
+ }
+ else
+ {
+ /* Add the cold_section to the unlikely chain of comdats. */
+ cold_section->comdat_group = kept->split_section->comdat_group;
+ kept->split_section->comdat_group = cold_section;
+ }
+ }
+ /* 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;
+ /* 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));
+ /* 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", section_name));
+ kept->split_section = section;
+ cold_section = section;
+ }
+ assert (cold_section != NULL && cold_section->comdat_group == NULL);
+ cold_section->is_split_cold_section = 1;
+ }
+ else
+ {
+ /* The function already exists, it must be a COMDAT. Only one
section
+ in the comdat group will be kept, we don't know which. Chain all
+ the comdat sections in the same comdat group to be emitted
+ together later. Keep one section as representative (kept) and
+ update its section_type to be equal to the type of the highest
+ priority section in the group. */
+
+ /* Two comdats in the same group can have different priorities. This
+ ensures that the "kept" comdat section has the priority of the
+ highest section in that comdat group. This is necessary because
+ the plugin does not know which section will be kept. */
+ if (section_priority[kept->section_type]
+ > section_priority[section_type])
+ kept->section_type = section_type;
+
+ section->comdat_group = kept->comdat_group;
+ kept->comdat_group = section;
+ }
}
}
@@ -1012,8 +1138,10 @@ get_layout (FILE *fp, void*** handles,
if (fp != NULL)
{
fprintf (fp, "%s entry count = %llu computed = %llu "
- "max count = %llu\n", s_it->full_name, s_it->weight,
- s_it->computed_weight, s_it->max_count);
+ "max count = %llu split = %d\n",
+ s_it->full_name, s_it->weight,
+ s_it->computed_weight, s_it->max_count,
+ s_it->is_split_cold_section);
}
s_it = s_it->group;
}
Index: function_reordering_plugin/callgraph.h
===================================================================
--- function_reordering_plugin/callgraph.h (revision 207700)
+++ function_reordering_plugin/callgraph.h (working copy)
@@ -236,6 +236,8 @@ typedef struct section_id_
is comdat hot and kept, pointer to the kept cold split
section. */
struct section_id_ *split_section;
+ /* If this is the cold part of a split section. */
+ char is_split_cold_section;
/* Check if this section has been considered for output. */
char processed;
} Section_id;
@@ -260,6 +262,7 @@ make_section_id (char *name, char *full_name,
s->computed_weight = 0;
s->max_count = 0;
s->split_section = NULL;
+ s->is_split_cold_section = 0;
return s;
}
