From patchwork Wed Jun 29 19:21:30 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 642188 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 3rfsxS23hKz9snl for ; Thu, 30 Jun 2016 05:22:03 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=RBt2W7it; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=LMcjrzJ6F7t3uchRk XVwUQ5WxOuxElHto65SGxmPaKwLj7SixprFY9aicZy1Jjo3KNtaWBsnwwyV0bvEz jBdQWij3CByqUBxr6b8zuLqZp/0U2ZKnjJ4RTP4j23T27cBB4rFRT1WP2O6QuiYX pJvXjB87Zkzncn216JkxO/LEwQ= 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:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=hODuIBQeFfDJpiKNbqmJ+QK ENqY=; b=RBt2W7it9gEn7DCIIvu7N4WPNe23FhFqypd4xnyEOGSvEN0DzAfU9rY PbriBmfAoz7OXcmR31YNKksTJDPNLLtHdet3WhqpvE0noZv5UuYn7dUdcPLPzTAH /peoZcWrfD/f6zSye4o85aTV5EZohJ+x5nBy0VJ/stI7R+95oOJw= Received: (qmail 106375 invoked by alias); 29 Jun 2016 19:21:53 -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 106352 invoked by uid 89); 29 Jun 2016 19:21:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=*node, AFAICT, 746, stamp X-HELO: mail-wm0-f54.google.com Received: from mail-wm0-f54.google.com (HELO mail-wm0-f54.google.com) (74.125.82.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 29 Jun 2016 19:21:42 +0000 Received: by mail-wm0-f54.google.com with SMTP id v199so193592012wmv.0 for ; Wed, 29 Jun 2016 12:21:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=0+l0LeS/ukWpcBmE9sHuLZ9vcm+0dvLMkYLCLRv0a3g=; b=cDFBpf7v5/VN14am3gMmyOPIxhBJDjVVS5k78Q8wjKk9g6R8DxG4/SRX0R5C3KjmrM bSyk8Tdns8AvylYTzRyhgICo0+ZeVkCf/SEc3fLKq7iH6u92JgLFMJJ/L+tKsF7KJtla fwyLJgJObkJ3Tshm7CEObKpjM+Bw+E3RqUjN9DX2y/E8p5GjKGshP/+M2GhnJP+f2AD7 auOvf76j4xITpTRD9Sxqrz2uEsro2EOkIwhke4bYKAiNvL5GLd6IWz1vjEzghApkflph tsnzzhHx71O7lX03JYCzm/yaDU+E6N5iie/mbJS7pPyHP0EEyVMkP6h2T3zSykxGJk0+ AjFg== X-Gm-Message-State: ALyK8tIZLMn4PNUBnHcUXL+22uBr3RBIaB/ltbkxPt+k+A764mh+/XmTusA78hppeelCiw== X-Received: by 10.194.192.193 with SMTP id hi1mr9838611wjc.4.1467228098546; Wed, 29 Jun 2016 12:21:38 -0700 (PDT) Received: from localhost (host81-140-212-11.range81-140.btcentralplus.com. [81.140.212.11]) by smtp.gmail.com with ESMTPSA id m5sm23686861wmm.10.2016.06.29.12.21.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 29 Jun 2016 12:21:37 -0700 (PDT) Date: Wed, 29 Jun 2016 20:21:30 +0100 From: Andrew Burgess To: Jeff Law Cc: gcc-patches@gcc.gnu.org, Jakub Jelinek Subject: Re: [PATCH 1/2] gcc: Remove unneeded global flag. Message-ID: <20160629192130.GF8823@embecosm.com> References: <512a967c-39c4-44f5-6f24-d75ef543979d@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <512a967c-39c4-44f5-6f24-d75ef543979d@redhat.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.6.1 (2016-04-27) X-IsSubscribed: yes * Jeff Law [2016-06-21 20:55:15 -0600]: > On 06/10/2016 10:56 AM, Andrew Burgess wrote: > > The global flag `user_defined_section_attribute' is set while parsing C > > code when the section attribute is encountered. The flag is set when > > anything has the section attribute applied to it, functions or data. > > > > The only place this global was used was within the gate function for > > partitioning blocks (pass_partition_blocks::gate), however, the > > partitioning is done during compilation, while the flag is set earlier, > > during the parsing. The flag is then cleared again during the final > > compilation pass. > > > > The result is that if any function or data has a section attribute then > > the flag will be set to true during the file parse pass. The first > > compiled function will then skip the partition-blocks pass, and the flag > > will be set back to false during the final-pass on the first function. > > After then, the flag is never set to true again. > > > > The guarding of the partition-blocks pass does not appear to be > > necessary, given that applying a section attribute correctly > > overrides the hot/cold section partitioning (this is taken care if in > > varasm.c). > > > > gcc/ChangeLog: > > > > * gcc/bb-reorder.c: Remove 'toplev.h' include. > > (pass_partition_blocks::gate): No longer check > > user_defined_section_attribute. > > * gcc/c-family/c-common.c (handle_section_attribute): No longer > > set user_defined_section_attribute. > > * gcc/final.c (rest_of_handle_final): Likewise. > > * gcc/toplev.c: Remove definition of user_defined_section_attribute. > > * gcc/toplev.h: Remove declaration of > > user_defined_section_attribute. > user_defined_section_attribute was introduced as part of the hot/cold > partitioning changes. > > https://gcc.gnu.org/ml/gcc-patches/2004-07/msg01545.html > > > What's supposed to happen is hot/cold partitioning is supposed to be turned > off for the function which has the a user defined section attribute. > > So proper behaviour is to set the flag to true when the attribute is parsed > and turn it off when we're finished with the current function. The gate for > hot/cold partitioning should check the value of the flag and avoid hot/cold > partitioning when the flag is true. > > So AFAICT everything is working as it should. Keep in mind that multiple > functions might have user defined section attributes. Jeff & Jakub, Thanks for taking the time to review and provide feedback. Let me explain how I _think_ it's working at the moment, then you can point out where I might be going wrong. My understanding is that currently all functions are parsed, and then all functions are optimised / compiled. The user_defined_section_attribute variable is initialised to false, and set true when the section attribute is parsed. However, as pass_partition_blocks::gate is called as part of the optimise / compile process we only read this variable once all of the functions have been parsed. The user_defined_section_attribute variable is reset to false in rest_of_handle_final, which is the final compile / optimise pass. So I think how it (doesn't) currently work is that user_defined_section_attribute starts as false, then if _any_ function has a user defined section attribute the variable is set true. Then, in the compiler / optimise phase the _first_ function will not perform the partition-blocks pass, after which user_defined_section_attribute will be reset to false, and all future blocks will go through the partition-blocks pass. In the original patch I simply removed user_defined_section_attribute figuring that given we've been happily running the partition-blocks pass then this can't be too harmful, however, as Jakub said perhaps I should have implemented a correct check. I've attached an updated patch to remove user_defined_section_attribute and replace the check in pass_partition_blocks::gate with one the looks for the section attribute on the function decl. Feedback welcome, Thanks, Andrew --- gcc: Remove unneeded global flag The global flag `user_defined_section_attribute' is set while parsing C code when the section attribute is encountered. The flag is set when anything has the section attribute applied to it, functions or data. The only place this global was used was within the gate function for partitioning blocks (pass_partition_blocks::gate), however, the partitioning is done during compilation, while the flag is set earlier, during the parsing. The flag is then cleared again during the final compilation pass. The result is that if any function or data has a section attribute then the flag will be set to true during the file parse pass. The first compiled function will then skip the partition-blocks pass, and the flag will be set back to false during the final-pass on the first function. After then, the flag is never set to true again. The guarding of the partition-blocks pass does not appear to be necessary, given that applying a section attribute correctly overrides the hot/cold section partitioning (this is taken care if in varasm.c). gcc/ChangeLog: * gcc/bb-reorder.c: Remove 'toplev.h' include. (pass_partition_blocks::gate): No longer check user_defined_section_attribute, instead check the function decl for a section attribute. * gcc/c-family/c-common.c (handle_section_attribute): No longer set user_defined_section_attribute. * gcc/final.c (rest_of_handle_final): Likewise. * gcc/toplev.c: Remove definition of user_defined_section_attribute. * gcc/toplev.h: Remove declaration of user_defined_section_attribute. --- gcc/bb-reorder.c | 3 +-- gcc/c-family/c-common.c | 2 -- gcc/final.c | 2 -- gcc/toplev.c | 5 ----- gcc/toplev.h | 5 ----- 5 files changed, 1 insertion(+), 16 deletions(-) diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c index bb8435f..0a482e6 100644 --- a/gcc/bb-reorder.c +++ b/gcc/bb-reorder.c @@ -106,7 +106,6 @@ #include "output.h" #include "expr.h" #include "params.h" -#include "toplev.h" /* user_defined_section_attribute */ #include "tree-pass.h" #include "cfgrtl.h" #include "cfganal.h" @@ -2890,7 +2889,7 @@ pass_partition_blocks::gate (function *fun) we are going to omit the reordering. */ && optimize_function_for_speed_p (fun) && !DECL_COMDAT_GROUP (current_function_decl) - && !user_defined_section_attribute); + && !lookup_attribute ("section", DECL_ATTRIBUTES (fun->decl))); } unsigned diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 3301c31..1f403d5 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -7619,8 +7619,6 @@ handle_section_attribute (tree *node, tree ARG_UNUSED (name), tree args, goto fail; } - user_defined_section_attribute = true; - if (!VAR_OR_FUNCTION_DECL_P (decl)) { error ("section attribute not allowed for %q+D", *node); diff --git a/gcc/final.c b/gcc/final.c index 5b04311..256ce34 100644 --- a/gcc/final.c +++ b/gcc/final.c @@ -4454,8 +4454,6 @@ rest_of_handle_final (void) assemble_end_function (current_function_decl, fnname); - user_defined_section_attribute = false; - /* Free up reg info memory. */ free_reg_info (); diff --git a/gcc/toplev.c b/gcc/toplev.c index da80097..648403e 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -148,11 +148,6 @@ HOST_WIDE_INT random_seed; the support provided depends on the backend. */ rtx stack_limit_rtx; -/* True if the user has tagged the function with the 'section' - attribute. */ - -bool user_defined_section_attribute = false; - struct target_flag_state default_target_flag_state; #if SWITCHABLE_TARGET struct target_flag_state *this_target_flag_state = &default_target_flag_state; diff --git a/gcc/toplev.h b/gcc/toplev.h index 06923cf..f62a172 100644 --- a/gcc/toplev.h +++ b/gcc/toplev.h @@ -74,11 +74,6 @@ extern void target_reinit (void); /* A unique local time stamp, might be zero if none is available. */ extern unsigned local_tick; -/* True if the user has tagged the function with the 'section' - attribute. */ - -extern bool user_defined_section_attribute; - /* See toplev.c. */ extern int flag_rerun_cse_after_global_opts;