From patchwork Wed Feb 12 00:51:57 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sriraman Tallam X-Patchwork-Id: 319449 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 C06022C0098 for ; Wed, 12 Feb 2014 11:52:09 +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=wsBtXv21+ZNdIZ+GBX W1wGFC76oPqnv3rydAfqObB7+NNwrWYCUVzoRW/5QzFl8GfHNCzHKPHsyNb1myLI GRZWe6o5frTlC/JvFRVkeQLPO4qwOdnlwX6ExLXm3eHlIREKsFllZKT3TaCEVSwP Mu99vUfFJaHtk3uUlkX52l2yc= 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=XnGzCM2+Lcw7IpbHHjUEOBYo MvQ=; b=Bs/GaPzisjl9yZH0aLenwDOhgrC9Q5LGXgNmGrjDNFfcHdnFiqTLObFm T9PehCNttSkdVcAbM8yUdubsPVjmi7qY8oQY9udd+XnCZNSt2lIGsowPW6R4h28D 8INnWxDYyS07tjaF0YtqJvO8f8zZE9LmqTrAcVAC3um8hfpef+E= Received: (qmail 29181 invoked by alias); 12 Feb 2014 00:52:02 -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 29053 invoked by uid 89); 12 Feb 2014 00:52:02 -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-vb0-f41.google.com Received: from mail-vb0-f41.google.com (HELO mail-vb0-f41.google.com) (209.85.212.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 12 Feb 2014 00:52:00 +0000 Received: by mail-vb0-f41.google.com with SMTP id g10so6494012vbg.14 for ; Tue, 11 Feb 2014 16:51:57 -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=1YEtM8L3XtVc79nt/YA/7VomldWPNHvJdgmSkpp4xDc=; b=CYX7Ypqx4xf3Z1PNIfY2qszNjjkBZlRmfQMVqPhYxQquK4ylpX1/kJfyyV+UBHQ+ou oNllARIX4WcvIWwO5hATEOTixFzNhBzrImC2UZ9uYe1BzGIxICHKDBLnhWCsn/T2ooEp NpVf2ajAw+5FI1S4e2nHzZdRggOOdj112lFP92jo8SxVbhzrUjfmQAb559hq2LOo2Mw7 jypg3A6c/F/IL8gCiFVEQcmfd1DhCSJBNaLLr6gnRAwLYxQH/wG/7z3HfSq8nS+Yu3Jo vEIcApubnL9GFa2bwSVqJte9MYg1vhyuWow/Kb6psYedPTNP0FbnmZY9+85XbQhIA6zJ ELYg== X-Gm-Message-State: ALoCoQkxufL/s2GFam/BzdC1k3zauZgcCW/SqAQCeNk9cCWKEAYCuzftn1zby5D1pG3ocXi40LSS+G63Pu80VpMpyDzKXLi4Vh9yCc9gjYf7KYGVgQhA4qEldEi/bXV+iUQ4KLasmyjGwvX9KYUb+eVsoGnfJF6kkjyOvlQxh3ASJBVcPc3emtfFlPJzg/UXq28+aaNAo/VVDKtDMFC/3TUHU2XhF37GxQ== MIME-Version: 1.0 X-Received: by 10.52.188.41 with SMTP id fx9mr25361579vdc.19.1392166317709; Tue, 11 Feb 2014 16:51:57 -0800 (PST) Received: by 10.52.32.233 with HTTP; Tue, 11 Feb 2014 16:51:57 -0800 (PST) In-Reply-To: References: Date: Tue, 11 Feb 2014 16:51:57 -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 , Xinliang David Li X-IsSubscribed: yes On Tue, Feb 11, 2014 at 4:39 PM, Teresa Johnson wrote: > On Tue, Feb 11, 2014 at 4:32 PM, Sriraman Tallam wrote: >> On Tue, Feb 11, 2014 at 3:29 PM, Teresa Johnson wrote: >>> >>> On Feb 11, 2014 2:37 PM, "Sriraman Tallam" wrote: >>>> >>>> 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 >>>> >> + 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 | tejohnson@google.com | 408-460-2413 > > > > -- > Teresa Johnson | Software Engineer | tejohnson@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; }