Message ID | 12fd0c18-79e0-d20d-2973-92639071c050@suse.cz |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E56E03857438 for <patchwork@sourceware.org>; Tue, 12 Oct 2021 12:17:51 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 48C5F3858430 for <gcc-patches@gcc.gnu.org>; Tue, 12 Oct 2021 12:17:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 48C5F3858430 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 0D6FF220E3 for <gcc-patches@gcc.gnu.org>; Tue, 12 Oct 2021 12:17:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1634041054; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=QHeYg9fSmY1KfT+2iiVDjpPvcWfftIeV2NZ0PqSgsYk=; b=f9pPHhDyj3YXblt3CQnTm8x2PzOYkc4jRXjlDUD1596kLhM2/tvLcDjW/MGZCRgebaYSoQ 3GOXQY7AvZKbn8qdzE1OkhdnfZmkp+eb0dYydQ0EwuaRHxvNRHhQwLq9mFfAAzoxLrsPM8 Zh9HWYXo55za/U6PaWPmBHdwVI8lpYQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1634041054; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=QHeYg9fSmY1KfT+2iiVDjpPvcWfftIeV2NZ0PqSgsYk=; b=Gi9aZPdsOUoBCF3RyphpA4mNxZJb/U8PjNqNBOlxm37POEbXygOQdaDDReOmi08f5hg3Pp hRqIv49KZZl30oBQ== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id EF76D13B6F for <gcc-patches@gcc.gnu.org>; Tue, 12 Oct 2021 12:17:33 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id IT41Od18ZWHyPgAAMHmgww (envelope-from <mliska@suse.cz>) for <gcc-patches@gcc.gnu.org>; Tue, 12 Oct 2021 12:17:33 +0000 Message-ID: <12fd0c18-79e0-d20d-2973-92639071c050@suse.cz> Date: Tue, 12 Oct 2021 14:17:33 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.2 From: =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz> Subject: [PATCH] Fix handling of flag_rename_registers. To: gcc-patches@gcc.gnu.org Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Fix handling of flag_rename_registers.
|
|
Commit Message
Martin Liška
Oct. 12, 2021, 12:17 p.m. UTC
Hello. The option is disabled in rs6000 target with: { OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 }, Thus, we have to do an auto-detection only if it's really unset and also equal to the Init value. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. And the problematic test-case works on ppc64le. Ready to be installed? Thanks, Martin PR target/102688 gcc/ChangeLog: * common.opt: Enable flag_rename_registers by default. * toplev.c (process_options): Auto-detect flag_rename_registers only if it is not turned off in a target. --- gcc/common.opt | 2 +- gcc/toplev.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)
Comments
On Tue, Oct 12, 2021 at 2:18 PM Martin Liška <mliska@suse.cz> wrote: > > Hello. > > The option is disabled in rs6000 target with: > > { OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 }, > > Thus, we have to do an auto-detection only if it's really unset and also > equal to the Init value. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > And the problematic test-case works on ppc64le. > > Ready to be installed? Hmm, I can see how it fixes the reported problem but I think the thing is fragile. I wonder if we can express things like + if (!OPTION_SET_P (flag_web)) flag_web = flag_unroll_loops; or + if (!OPTION_SET_P (flag_rename_registers)) flag_rename_registers = flag_unroll_loops; by adding EnabledBy(funroll-loops) to the respective options instead (and funroll-loops EnabledBy(funroll-all-loops)) All SET_OPTION_IF_UNSET are fragile with respect to target overrides (-fprofile-use does a lot of those for example). I suppose opts_set could also record whether the target overrided sth with its option_optimization_table. Richard. > Thanks, > Martin > > PR target/102688 > > gcc/ChangeLog: > > * common.opt: Enable flag_rename_registers by default. > * toplev.c (process_options): Auto-detect flag_rename_registers > only if it is not turned off in a target. > --- > gcc/common.opt | 2 +- > gcc/toplev.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/gcc/common.opt b/gcc/common.opt > index 4099effcc80..2c6be1bdd36 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -2399,7 +2399,7 @@ Common Var(flag_live_range_shrinkage) Init(0) Optimization > Relief of register pressure through live range shrinkage. > > frename-registers > -Common Var(flag_rename_registers) Optimization > +Common Var(flag_rename_registers) Init(1) Optimization > Perform a register renaming optimization pass. > > fschedule-fusion > diff --git a/gcc/toplev.c b/gcc/toplev.c > index 167feac2583..ee7d8854f90 100644 > --- a/gcc/toplev.c > +++ b/gcc/toplev.c > @@ -1335,7 +1335,8 @@ process_options (bool no_backend) > if (!OPTION_SET_P (flag_web)) > flag_web = flag_unroll_loops; > > - if (!OPTION_SET_P (flag_rename_registers)) > + /* The option can be turned off in a target. */ > + if (!OPTION_SET_P (flag_rename_registers) && flag_rename_registers) > flag_rename_registers = flag_unroll_loops; > > if (flag_non_call_exceptions) > -- > 2.33.0 >
On 10/12/21 15:37, Richard Biener wrote: > On Tue, Oct 12, 2021 at 2:18 PM Martin Liška <mliska@suse.cz> wrote: >> >> Hello. >> >> The option is disabled in rs6000 target with: >> >> { OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 }, >> >> Thus, we have to do an auto-detection only if it's really unset and also >> equal to the Init value. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> And the problematic test-case works on ppc64le. >> >> Ready to be installed? > > Hmm, I can see how it fixes the reported problem but I think the > thing is fragile. You are fully right, it's quite fragile. > I wonder if we can express things like > > + if (!OPTION_SET_P (flag_web)) > flag_web = flag_unroll_loops; > > or > > + if (!OPTION_SET_P (flag_rename_registers)) > flag_rename_registers = flag_unroll_loops; > > by adding EnabledBy(funroll-loops) to the respective options instead > (and funroll-loops EnabledBy(funroll-all-loops)) Testing that approach, I like it. Note that my fix: if (!OPTION_SET_P (flag_rename_registers) && flag_rename_registers) won't work if one target sets flag_rename_registers = 1 and another to flag_rename_registers = 0. Then one can't use Init setting a proper default value. > > All SET_OPTION_IF_UNSET are fragile with respect to target overrides > (-fprofile-use does a lot of those for example). > > I suppose opts_set could also record whether the target overrided > sth with its option_optimization_table. I can experiment with a patch where SET_OPTION_IF_UNSET modified opts_set. Thanks for clever feedback. Martin > > Richard. > >> Thanks, >> Martin >> >> PR target/102688 >> >> gcc/ChangeLog: >> >> * common.opt: Enable flag_rename_registers by default. >> * toplev.c (process_options): Auto-detect flag_rename_registers >> only if it is not turned off in a target. >> --- >> gcc/common.opt | 2 +- >> gcc/toplev.c | 3 ++- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/common.opt b/gcc/common.opt >> index 4099effcc80..2c6be1bdd36 100644 >> --- a/gcc/common.opt >> +++ b/gcc/common.opt >> @@ -2399,7 +2399,7 @@ Common Var(flag_live_range_shrinkage) Init(0) Optimization >> Relief of register pressure through live range shrinkage. >> >> frename-registers >> -Common Var(flag_rename_registers) Optimization >> +Common Var(flag_rename_registers) Init(1) Optimization >> Perform a register renaming optimization pass. >> >> fschedule-fusion >> diff --git a/gcc/toplev.c b/gcc/toplev.c >> index 167feac2583..ee7d8854f90 100644 >> --- a/gcc/toplev.c >> +++ b/gcc/toplev.c >> @@ -1335,7 +1335,8 @@ process_options (bool no_backend) >> if (!OPTION_SET_P (flag_web)) >> flag_web = flag_unroll_loops; >> >> - if (!OPTION_SET_P (flag_rename_registers)) >> + /* The option can be turned off in a target. */ >> + if (!OPTION_SET_P (flag_rename_registers) && flag_rename_registers) >> flag_rename_registers = flag_unroll_loops; >> >> if (flag_non_call_exceptions) >> -- >> 2.33.0 >>
On 10/12/21 15:37, Richard Biener wrote: > by adding EnabledBy(funroll-loops) to the respective options instead > (and funroll-loops EnabledBy(funroll-all-loops)) All right, so the suggested approach works correctly. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
On Tue, Oct 12, 2021 at 5:11 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/12/21 15:37, Richard Biener wrote: > > by adding EnabledBy(funroll-loops) to the respective options instead > > (and funroll-loops EnabledBy(funroll-all-loops)) > > All right, so the suggested approach works correctly. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? funroll-all-loops -Common Var(flag_unroll_all_loops) Optimization +Common Var(flag_unroll_all_loops) Optimization EnabledBy(funroll-all-loops) that should be on funroll-loops? Can you verify that the two-step -funroll-all-loops -> -funroll-loops -> -frename-registers works and that it is not somehow dependent on ordering? Otherwise we have to use EnabledBy(funroll-loops,funroll-all-loops) on frename-registers. I guess the EnabledBy doesn't work if the target decides to set flag_unroll_loop in one of its hooks rather than via its option table override? (as said, this is all a mess...) But grep should be your friend telling whether any target overrides any of the flags... I do hope we can eventually reduce the number of pre-/post-/lang/target/common processing phases for options :/ Meh. Richard. > Thanks, > Martin
On 10/13/21 10:39, Richard Biener wrote: > On Tue, Oct 12, 2021 at 5:11 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 10/12/21 15:37, Richard Biener wrote: >>> by adding EnabledBy(funroll-loops) to the respective options instead >>> (and funroll-loops EnabledBy(funroll-all-loops)) >> >> All right, so the suggested approach works correctly. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? > > funroll-all-loops > -Common Var(flag_unroll_all_loops) Optimization > +Common Var(flag_unroll_all_loops) Optimization EnabledBy(funroll-all-loops) > > that should be on funroll-loops? Yeah, what a stupid error. > > Can you verify that the two-step -funroll-all-loops -> -funroll-loops > -> -frename-registers Yes, verified that in debugger, it's not dependent on an ordering. > works and that it is not somehow dependent on ordering? Otherwise we have to > use EnabledBy(funroll-loops,funroll-all-loops) on frename-registers. > I guess the > EnabledBy doesn't work if the target decides to set flag_unroll_loop in one of > its hooks rather than via its option table override? (as said, this > is all a mess...) It's a complete mess. The only override happens in rs6000_override_options_after_change. I think it can also utilize EnabledBy, but I would like to do it in a different patch. > > But grep should be your friend telling whether any target overrides > any of the flags... > > I do hope we can eventually reduce the number of pre-/post-/lang/target/common > processing phases for options :/ Meh. Huh. May I install this fixed patch once it's tested? Martin > > Richard. > >> Thanks, >> Martin
On Wed, Oct 13, 2021 at 12:02 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/13/21 10:39, Richard Biener wrote: > > On Tue, Oct 12, 2021 at 5:11 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> On 10/12/21 15:37, Richard Biener wrote: > >>> by adding EnabledBy(funroll-loops) to the respective options instead > >>> (and funroll-loops EnabledBy(funroll-all-loops)) > >> > >> All right, so the suggested approach works correctly. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> Ready to be installed? > > > > funroll-all-loops > > -Common Var(flag_unroll_all_loops) Optimization > > +Common Var(flag_unroll_all_loops) Optimization EnabledBy(funroll-all-loops) > > > > that should be on funroll-loops? > > Yeah, what a stupid error. > > > > > Can you verify that the two-step -funroll-all-loops -> -funroll-loops > > -> -frename-registers > > Yes, verified that in debugger, it's not dependent on an ordering. > > > works and that it is not somehow dependent on ordering? Otherwise we have to > > use EnabledBy(funroll-loops,funroll-all-loops) on frename-registers. > > I guess the > > EnabledBy doesn't work if the target decides to set flag_unroll_loop in one of > > its hooks rather than via its option table override? (as said, this > > is all a mess...) > > It's a complete mess. The only override happens in > rs6000_override_options_after_change. I think it can also utilize EnabledBy, but > I would like to do it in a different patch. > > > > > But grep should be your friend telling whether any target overrides > > any of the flags... > > > > I do hope we can eventually reduce the number of pre-/post-/lang/target/common > > processing phases for options :/ Meh. > > Huh. > > May I install this fixed patch once it's tested? Yes please. Thanks, Richard. > Martin > > > > > Richard. > > > >> Thanks, > >> Martin >
On 10/13/2021 4:02 AM, Martin Liška wrote: > >> works and that it is not somehow dependent on ordering? Otherwise we >> have to >> use EnabledBy(funroll-loops,funroll-all-loops) on frename-registers. >> I guess the >> EnabledBy doesn't work if the target decides to set flag_unroll_loop >> in one of >> its hooks rather than via its option table override? (as said, this >> is all a mess...) > > It's a complete mess. The only override happens in > rs6000_override_options_after_change. I think it can also utilize > EnabledBy, but > I would like to do it in a different patch. So what's the preferred way to handle this? We're in the process of evaluating -frename-registers on our target right now and subject to verification of a couple issues, our inclination is to turn it on for our target at -O2. Jeff
On 10/14/21 16:27, Jeff Law wrote: > So what's the preferred way to handle this? We're in the process of evaluating -frename-registers on our target right now and subject to verification of a couple issues, our inclination is to turn it on for our target at -O2. > > Jeff I think the best approach is doing that in TARGET_OPTION_OPTIMIZATION_TABLE like c6x does: static const struct default_options c6x_option_optimization_table[] = { { OPT_LEVELS_1_PLUS, OPT_frename_registers, NULL, 1 }, ... } Cheers, Martin
On 10/15/2021 9:27 AM, Martin Liška wrote: > On 10/14/21 16:27, Jeff Law wrote: >> So what's the preferred way to handle this? We're in the process of >> evaluating -frename-registers on our target right now and subject to >> verification of a couple issues, our inclination is to turn it on for >> our target at -O2. >> >> Jeff > > I think the best approach is doing that in > TARGET_OPTION_OPTIMIZATION_TABLE like c6x does: > > static const struct default_options c6x_option_optimization_table[] = > { > { OPT_LEVELS_1_PLUS, OPT_frename_registers, NULL, 1 }, > ... > } ACK. So nothing particularly special :-) jeff
diff --git a/gcc/common.opt b/gcc/common.opt index 4099effcc80..2c6be1bdd36 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -2399,7 +2399,7 @@ Common Var(flag_live_range_shrinkage) Init(0) Optimization Relief of register pressure through live range shrinkage. frename-registers -Common Var(flag_rename_registers) Optimization +Common Var(flag_rename_registers) Init(1) Optimization Perform a register renaming optimization pass. fschedule-fusion diff --git a/gcc/toplev.c b/gcc/toplev.c index 167feac2583..ee7d8854f90 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1335,7 +1335,8 @@ process_options (bool no_backend) if (!OPTION_SET_P (flag_web)) flag_web = flag_unroll_loops; - if (!OPTION_SET_P (flag_rename_registers)) + /* The option can be turned off in a target. */ + if (!OPTION_SET_P (flag_rename_registers) && flag_rename_registers) flag_rename_registers = flag_unroll_loops; if (flag_non_call_exceptions)