From patchwork Tue Feb 11 22:37:13 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sriraman Tallam X-Patchwork-Id: 319431 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3336E2C009B for ; Wed, 12 Feb 2014 09:37:26 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=y4Lp/cZeQsXsN6ZBKi 3hgk1TmCepB+/uM8kJpx1SHBrlhSHJ9GZvJ5tMdMbIevAWJ4HKssM1Cam8vfCYWX AgKnA9ixAWcOMo67fuXGe4yZNggubPOUQ8E4yfIsFOIgNt2lY/U4O75RfEMdAPKu UU96o6PtDjuVA4jhF5CLPt/rs= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=h1sEvC6aMALSxd4dt7AXmTkL 6V4=; b=kO3C6xUDbWGGM13IP/H+OtvBsvmTidyusKeyEFozIeRU2F9Kuq2skMSJ 36zar3iE8QQXr0xbfHRlk9jJrvjVqluDA1B+ICpF0rV0qHdYP6CWTOOeos167EeC t2rGqyt7nqwfFxkgQ1Esmb8xCTW9joYo5jwclbHltKWeqD+xEMk= Received: (qmail 29288 invoked by alias); 11 Feb 2014 22:37:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 29279 invoked by uid 89); 11 Feb 2014 22:37:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-vc0-f173.google.com Received: from mail-vc0-f173.google.com (HELO mail-vc0-f173.google.com) (209.85.220.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 11 Feb 2014 22:37:16 +0000 Received: by mail-vc0-f173.google.com with SMTP id ld13so6313405vcb.4 for ; Tue, 11 Feb 2014 14:37:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=xqezEDJS9dFXGMTA9c3JgYTsQ+UeXM2YYZQxi8hiiJM=; b=kLxyjjHuQvjvjtwTiaa38J3S7+qF7IGDpbax7Y2rwjMux7ozk2sYMhdyk9xj4KT5Pk rZKjEeTrimqgOq7lGBG28czxE54zZ+mjIuc42yxWVxRbfANdnJkSNvNZA9rspn/RHFGk D4fKzyNX/VlNN9GfPdfecdfK5irqPvXRIZioO0YGHBB/UHsxy++PwMPBCQMbZeJDc/kV 906ge+0QrPx9r/sHC0fxc+qdOhSYJ9bMuzIVqC38zQs60J/GMbuea65mAfT2jAREUXlS +cX736clQvYJSxurnnCT8edEmBAYzBGeO1MqG7yhH1WmFyx+SIat2P56qdeUI0I6O/qH v4kA== X-Gm-Message-State: ALoCoQmrhKcBLkOW3m7NSDo0E7Z90KaQ1LTu3mcoj0y6OX+RvTQ/4Kc4qauDOQvrZSPkm89SywjsjJyvKuKCN7IhEOKhTBAMp9CTN52iiNRIPggfHRj3jnALE86AjZ0ar5KEVw5bp80zGYcDrSDVsse32eYvkvcK6kia8eQP2blHvhULo0/+rQImFbLkNFToF7LPsyiNw4WeYlTd45Bfg/9Sez8dtKKP/A== MIME-Version: 1.0 X-Received: by 10.52.164.39 with SMTP id yn7mr10678776vdb.25.1392158233533; Tue, 11 Feb 2014 14:37:13 -0800 (PST) Received: by 10.52.32.233 with HTTP; Tue, 11 Feb 2014 14:37:13 -0800 (PST) In-Reply-To: References: Date: Tue, 11 Feb 2014 14:37:13 -0800 Message-ID: Subject: Re: [google][gcc-4_8][patch]Handle Split functions in the Function Reordering Plugin From: Sriraman Tallam To: Teresa Johnson Cc: GCC Patches , David Li X-IsSubscribed: yes On Tue, Feb 11, 2014 at 1:32 PM, Teresa Johnson wrote: > On Mon, Feb 10, 2014 at 7:15 PM, Sriraman Tallam 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 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; } 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,129 @@ 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)) + { + 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 + { + 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", 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; + } + 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 +1136,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: 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: 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" } } */