Message ID | 12fd0c18-79e0-d20d-2973-92639071c050@suse.cz |
---|---|
State | New |
Headers | show |
Series | Fix handling of flag_rename_registers. | expand |
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)