From patchwork Wed Feb 12 00:32:34 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sriraman Tallam X-Patchwork-Id: 319447 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 12F0D2C00A2 for ; Wed, 12 Feb 2014 11:32:46 +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=L4XK4lcZT5wZ+p88kN g2wXIUsxI2t6hZQqyRmoPJfe+BA+xQSsuFBuSVxG7rHFvXi+R4Dttj550ov7AxVP h6nh7z9Tby2GdW8OtAlkNwmha0rLFcAKk8DtmzPgfrES2Mfq8ffUG0325fw9emtm vSN5RM8rnGQ5M44XCHCcS7904= 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=2lAeTzybcdw/eRE49ZZifZ/4 A1g=; b=Qz4y0SFGsB+4mSssDYmV/wwtpuq8wORLfxXo11Ksk3PIk7MCgiBsd4z5 6LPvZSeNnMrZCGOpHFAmtUOPQUMaBRtAiOp0L6WubNsf42srYqYGN81U8VewbLKN eL2jeXckTfxoEB+40PPpWfrZWEs38hbuq5eAURm5/ES6lJsRnA0= Received: (qmail 19938 invoked by alias); 12 Feb 2014 00:32:39 -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 19888 invoked by uid 89); 12 Feb 2014 00:32:39 -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-f179.google.com Received: from mail-vc0-f179.google.com (HELO mail-vc0-f179.google.com) (209.85.220.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 12 Feb 2014 00:32:37 +0000 Received: by mail-vc0-f179.google.com with SMTP id lh14so6408580vcb.24 for ; Tue, 11 Feb 2014 16:32:35 -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=FL9X0JwFStiknRj5Ivcz3ViqOvtHAnoo8d2TJSq8dCE=; b=Dig4uS4XcJhL5lpRCj0oDe8I8TnD13gp7WKDCwqFOPDz0Z+4qr3UKbs3f7mIeBTwc8 MbiyMx8xe8jC1JXRKNKVQoSrmBYLumFdy4LckvWMlCF736MHzRwf5s1xgYnZxi53nnZK Y6xNo6mS9B3YojXuxy4BXDpqktzdNeL45q0k/39OQhezIEbbrzLtDIADWIrBJSjY1+8G /N2c+va0aE3NTVBpRC+R35EhybwFIds4/MzhM5h3RnDrwSOERI91DvDYH4RCTVS3nqgU 7t5xQGaFb0Ola7j+2rlJDSSYdYTwJxshDLmxKhKU0+hNS069TK2Cle3eLj0oMxK3/9ft MHIw== X-Gm-Message-State: ALoCoQnCnPeQ4TpzO/SHg9UkEpWs4LsQSNTecJcTH1xPUXclne40gbtjzNzqRkmUArdqodKCyvMI5M0Hzhy898xM4B1q5ady5aKyKP7+4lLs42rs5mSHgk805s32Ufwws2ifBeX0mUwiTCNU9D/0KH/xzWGmntqB9cEbqA+2h93kAgLW1+mWxe0iZ9i/NeLpbcu/QrRDkv3U0vSD7l4eVCXlAXm4J7WGzQ== MIME-Version: 1.0 X-Received: by 10.58.211.130 with SMTP id nc2mr29543421vec.7.1392165154820; Tue, 11 Feb 2014 16:32:34 -0800 (PST) Received: by 10.52.32.233 with HTTP; Tue, 11 Feb 2014 16:32:34 -0800 (PST) In-Reply-To: References: Date: Tue, 11 Feb 2014 16:32:34 -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 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. 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 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" } } */ 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,128 @@ 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", 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 +1135,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; }