From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id E723F3858414 for ; Tue, 19 Oct 2021 14:12:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E723F3858414 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 19JEBEhN022809; Tue, 19 Oct 2021 09:11:14 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 19JEBEE6022808; Tue, 19 Oct 2021 09:11:14 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 19 Oct 2021 09:11:13 -0500 From: Segher Boessenkool To: Martin =?utf-8?B?TGnFoWth?= Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] rs6000: Remove unnecessary option manipulation. Message-ID: <20211019141113.GO614@gate.crashing.org> References: <2f57b5b5-3894-29b4-0e20-725bf273b496@suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2f57b5b5-3894-29b4-0e20-725bf273b496@suse.cz> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Oct 2021 14:12:16 -0000 Hi! On Thu, Oct 14, 2021 at 09:49:30AM +0200, Martin Liška wrote: > gcc/ChangeLog: > * config/rs6000/rs6000.c (rs6000_override_options_after_change): > Do not set flag_rename_registers, it's already default behavior. It defaults to *off*? frename-registers Common Var(flag_rename_registers) Optimization EnabledBy(funroll-loops) Perform a register renaming optimization pass. > Use EnabledBy for unroll_only_small_loops. > * config/rs6000/rs6000.opt: Use EnabledBy for > unroll_only_small_loops. > --- > gcc/config/rs6000/rs6000.c | 7 +------ > gcc/config/rs6000/rs6000.opt | 2 +- > 2 files changed, 2 insertions(+), 7 deletions(-) > > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index acba4d9f26c..40146179e06 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -3472,13 +3472,8 @@ rs6000_override_options_after_change (void) > /* Explicit -funroll-loops turns -munroll-only-small-loops off, and > turns -frename-registers on. */ > if ((OPTION_SET_P (flag_unroll_loops) && flag_unroll_loops) > - || (OPTION_SET_P (flag_unroll_all_loops) > - && flag_unroll_all_loops)) > + || (OPTION_SET_P (flag_unroll_all_loops && flag_unroll_all_loops))) That doesn't do what the changelog said, and it is not obvious at all that this is correct? Maybe this works with the current implementation of that macro (I assume you tested it works :-) ), but that is not something you can depend on. This expands to global_options_set.x_flag_unroll_all_loops && flag_unroll_all_loops just like the previous code did, but that one was obvious, and this is not. > { > - if (!OPTION_SET_P (unroll_only_small_loops)) > - unroll_only_small_loops = 0; > - if (!OPTION_SET_P (flag_rename_registers)) > - flag_rename_registers = 1; > if (!OPTION_SET_P (flag_cunroll_grow_size)) > flag_cunroll_grow_size = 1; > } > diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt > index 9d7878f144a..faeb7423ca7 100644 > --- a/gcc/config/rs6000/rs6000.opt > +++ b/gcc/config/rs6000/rs6000.opt > @@ -546,7 +546,7 @@ Target Undocumented Var(rs6000_optimize_swaps) Init(1) > Save > Analyze and remove doubleword swaps from VSX computations. > > munroll-only-small-loops > -Target Undocumented Var(unroll_only_small_loops) Init(0) Save > +Target Undocumented Var(unroll_only_small_loops) Init(0) Save > EnabledBy(funroll-loops) Your patches cannot apply. Please send them non-wordwrapped. This isn't the endpoint of the changes here I hope? The macro games make everything less readable (so, harder to change) and more fragile. Segher