* [PATCH] rs6000: Remove unnecessary option manipulation. @ 2021-10-14 7:49 Martin Liška 2021-10-14 15:10 ` Bill Schmidt 2021-10-19 14:11 ` Segher Boessenkool 0 siblings, 2 replies; 10+ messages in thread From: Martin Liška @ 2021-10-14 7:49 UTC (permalink / raw) To: gcc-patches Hello. There's follow up flag handling simplification based on 4ab51fa0e1705201420d87b601bd92bc643b3d52. Patch can bootstrap on ppc64le-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/ChangeLog: * config/rs6000/rs6000.c (rs6000_override_options_after_change): Do not set flag_rename_registers, it's already default behavior. 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))) { - 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) ; Use conservative small loop unrolling. mpower9-misc -- 2.33.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-14 7:49 [PATCH] rs6000: Remove unnecessary option manipulation Martin Liška @ 2021-10-14 15:10 ` Bill Schmidt 2021-10-15 15:24 ` Martin Liška 2021-10-19 14:11 ` Segher Boessenkool 1 sibling, 1 reply; 10+ messages in thread From: Bill Schmidt @ 2021-10-14 15:10 UTC (permalink / raw) To: gcc-patches Hi Martin, On 10/14/21 2:49 AM, Martin Liška wrote: > Hello. > > There's follow up flag handling simplification based on > 4ab51fa0e1705201420d87b601bd92bc643b3d52. > > Patch can bootstrap on ppc64le-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > gcc/ChangeLog: > > * config/rs6000/rs6000.c (rs6000_override_options_after_change): > Do not set flag_rename_registers, it's already default behavior. > 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))) Looks like you got your parentheses wrong here. Thanks, Bill > { > - 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) > ; Use conservative small loop unrolling. > > mpower9-misc ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-14 15:10 ` Bill Schmidt @ 2021-10-15 15:24 ` Martin Liška 2021-10-19 8:53 ` Martin Liška 2021-10-19 14:23 ` Segher Boessenkool 0 siblings, 2 replies; 10+ messages in thread From: Martin Liška @ 2021-10-15 15:24 UTC (permalink / raw) To: wschmidt, gcc-patches [-- Attachment #1: Type: text/plain, Size: 218 bytes --] On 10/14/21 17:10, Bill Schmidt via Gcc-patches wrote: > Looks like you got your parentheses wrong here. Whoops, thanks for the heads up. I'm testing this fixed version. P.S. Next time, please CC me. Thanks, Martin [-- Attachment #2: 0001-rs6000-Remove-unnecessary-option-manipulation.patch --] [-- Type: text/x-patch, Size: 2007 bytes --] From cd9891ec3eed3a5b289b7c556598606d21e48206 Mon Sep 17 00:00:00 2001 From: Martin Liska <mliska@suse.cz> Date: Wed, 13 Oct 2021 14:20:33 +0200 Subject: [PATCH] rs6000: Remove unnecessary option manipulation. gcc/ChangeLog: * config/rs6000/rs6000.c (rs6000_override_options_after_change): Do not set flag_rename_registers, it's already default behavior. 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 01a95591a5d..b9dddcd0aa1 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)) { - 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) ; Use conservative small loop unrolling. mpower9-misc -- 2.33.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-15 15:24 ` Martin Liška @ 2021-10-19 8:53 ` Martin Liška 2021-10-19 13:07 ` Bill Schmidt 2021-10-19 14:23 ` Segher Boessenkool 1 sibling, 1 reply; 10+ messages in thread From: Martin Liška @ 2021-10-19 8:53 UTC (permalink / raw) To: wschmidt, gcc-patches On 10/15/21 17:24, Martin Liška wrote: > P.S. Next time, please CC me. All right, I tested the updated patch and it works fine on ppc64le-linux-gnu. May I install it? Thanks, Martin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-19 8:53 ` Martin Liška @ 2021-10-19 13:07 ` Bill Schmidt 0 siblings, 0 replies; 10+ messages in thread From: Bill Schmidt @ 2021-10-19 13:07 UTC (permalink / raw) To: Martin Liška, gcc-patches, Segher Boessenkool, David Edelsohn Hi Martin, On 10/19/21 3:53 AM, Martin Liška wrote: > On 10/15/21 17:24, Martin Liška wrote: >> P.S. Next time, please CC me. > > All right, I tested the updated patch and it works fine > on ppc64le-linux-gnu. > > May I install it? I'm not a maintainer, so can't approve -- CCing those who can. Thanks! Bill > Thanks, > Martin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-15 15:24 ` Martin Liška 2021-10-19 8:53 ` Martin Liška @ 2021-10-19 14:23 ` Segher Boessenkool 2021-10-19 14:43 ` Martin Liška 1 sibling, 1 reply; 10+ messages in thread From: Segher Boessenkool @ 2021-10-19 14:23 UTC (permalink / raw) To: Martin Liška; +Cc: wschmidt, gcc-patches On Fri, Oct 15, 2021 at 05:24:32PM +0200, Martin Liška wrote: > On 10/14/21 17:10, Bill Schmidt via Gcc-patches wrote: > >Looks like you got your parentheses wrong here. > > Whoops, thanks for the heads up. > > I'm testing this fixed version. Please start a new thread for every new patch (series). I missed this one like this, instead I reviewed the older one. [-- Attachment #2: 0001-rs6000-Remove-unnecessary-option-manipulation.patch --] [-- Type: text/x-patch, Encoding: base64, Size: 2.6K --] Don't encode as non-text. Don't use x-anything if you can help it, it is meaningless in email. Use git send-email, it makes everything work :-) Segher ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-19 14:23 ` Segher Boessenkool @ 2021-10-19 14:43 ` Martin Liška 2021-11-04 10:08 ` Martin Liška 2021-11-04 11:38 ` Segher Boessenkool 0 siblings, 2 replies; 10+ messages in thread From: Martin Liška @ 2021-10-19 14:43 UTC (permalink / raw) To: Segher Boessenkool; +Cc: wschmidt, gcc-patches [-- Attachment #1: Type: text/plain, Size: 1163 bytes --] On 10/19/21 16:23, Segher Boessenkool wrote: > On Fri, Oct 15, 2021 at 05:24:32PM +0200, Martin Liška wrote: >> On 10/14/21 17:10, Bill Schmidt via Gcc-patches wrote: >>> Looks like you got your parentheses wrong here. >> >> Whoops, thanks for the heads up. >> >> I'm testing this fixed version. > > Please start a new thread for every new patch (series). I missed this > one like this, instead I reviewed the older one. Is it really best practice. My impression is that patch review (iterating over a patch) happens in the same thread (in most cases). It's caused by discussion in between sender reviewers. > > [-- Attachment #2: 0001-rs6000-Remove-unnecessary-option-manipulation.patch --] > [-- Type: text/x-patch, Encoding: base64, Size: 2.6K --] Meh :) If I need a reply to somebody's questions, I always attach patch as an attachment. And I can't likely influence how Thunderbird is going to mark it. > > Don't encode as non-text. Don't use x-anything if you can help it, it > is meaningless in email. > > Use git send-email, it makes everything work :-) I know. Anyway, sending updated version of the patch. Cheers, Martin > > > Segher > [-- Attachment #2: 0001-rs6000-Remove-unnecessary-option-manipulation.patch --] [-- Type: text/x-patch, Size: 2029 bytes --] From d32d9445d8ec868abc965f167fffa10ab19417e5 Mon Sep 17 00:00:00 2001 From: Martin Liska <mliska@suse.cz> Date: Wed, 13 Oct 2021 14:20:33 +0200 Subject: [PATCH] rs6000: Remove unnecessary option manipulation. gcc/ChangeLog: * config/rs6000/rs6000.c (rs6000_override_options_after_change): Do not set flag_rename_registers, it's already enabled with EnabledBy(funroll-loops). 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 01a95591a5d..b9dddcd0aa1 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)) { - 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) ; Use conservative small loop unrolling. mpower9-misc -- 2.33.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-19 14:43 ` Martin Liška @ 2021-11-04 10:08 ` Martin Liška 2021-11-04 11:38 ` Segher Boessenkool 1 sibling, 0 replies; 10+ messages in thread From: Martin Liška @ 2021-11-04 10:08 UTC (permalink / raw) To: Segher Boessenkool; +Cc: gcc-patches, wschmidt @Segher: PING On 10/19/21 16:43, Martin Liška wrote: > On 10/19/21 16:23, Segher Boessenkool wrote: >> On Fri, Oct 15, 2021 at 05:24:32PM +0200, Martin Liška wrote: >>> On 10/14/21 17:10, Bill Schmidt via Gcc-patches wrote: >>>> Looks like you got your parentheses wrong here. >>> >>> Whoops, thanks for the heads up. >>> >>> I'm testing this fixed version. >> >> Please start a new thread for every new patch (series). I missed this >> one like this, instead I reviewed the older one. > > Is it really best practice. My impression is that patch review (iterating over > a patch) happens in the same thread (in most cases). It's caused by discussion > in between sender reviewers. > >> >> [-- Attachment #2: 0001-rs6000-Remove-unnecessary-option-manipulation.patch --] >> [-- Type: text/x-patch, Encoding: base64, Size: 2.6K --] > > Meh :) If I need a reply to somebody's questions, I always attach patch as an attachment. > And I can't likely influence how Thunderbird is going to mark it. > >> >> Don't encode as non-text. Don't use x-anything if you can help it, it >> is meaningless in email. >> >> Use git send-email, it makes everything work :-) > > I know. > > Anyway, sending updated version of the patch. > > Cheers, > Martin > >> >> >> Segher >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-19 14:43 ` Martin Liška 2021-11-04 10:08 ` Martin Liška @ 2021-11-04 11:38 ` Segher Boessenkool 1 sibling, 0 replies; 10+ messages in thread From: Segher Boessenkool @ 2021-11-04 11:38 UTC (permalink / raw) To: Martin Liška; +Cc: wschmidt, gcc-patches On Tue, Oct 19, 2021 at 04:43:40PM +0200, Martin Liška wrote: > On 10/19/21 16:23, Segher Boessenkool wrote: > >On Fri, Oct 15, 2021 at 05:24:32PM +0200, Martin Liška wrote: > >>On 10/14/21 17:10, Bill Schmidt via Gcc-patches wrote: > >>>Looks like you got your parentheses wrong here. > >> > >>Whoops, thanks for the heads up. > >> > >>I'm testing this fixed version. > > > >Please start a new thread for every new patch (series). I missed this > >one like this, instead I reviewed the older one. > > Is it really best practice. My impression is that patch review (iterating > over > a patch) happens in the same thread (in most cases). It's caused by > discussion > in between sender reviewers. Yes, it is best practice. It is impossible to juggle multiple versions of a patch at once and not have some fall on the floor. > >[-- Attachment #2: > >0001-rs6000-Remove-unnecessary-option-manipulation.patch --] > >[-- Type: text/x-patch, Encoding: base64, Size: 2.6K --] > > Meh :) If I need a reply to somebody's questions, I always attach patch as > an attachment. > And I can't likely influence how Thunderbird is going to mark it. You should not use base64. This is documented. Patches in the archive will not show up either that way. > Anyway, sending updated version of the patch. Not in a reply please. If nothing else, this makes it hard for other people to apply your patches (to test them out, or to actually commit them upstream). Segher ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rs6000: Remove unnecessary option manipulation. 2021-10-14 7:49 [PATCH] rs6000: Remove unnecessary option manipulation Martin Liška 2021-10-14 15:10 ` Bill Schmidt @ 2021-10-19 14:11 ` Segher Boessenkool 1 sibling, 0 replies; 10+ messages in thread From: Segher Boessenkool @ 2021-10-19 14:11 UTC (permalink / raw) To: Martin Liška; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-04 11:39 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-14 7:49 [PATCH] rs6000: Remove unnecessary option manipulation Martin Liška 2021-10-14 15:10 ` Bill Schmidt 2021-10-15 15:24 ` Martin Liška 2021-10-19 8:53 ` Martin Liška 2021-10-19 13:07 ` Bill Schmidt 2021-10-19 14:23 ` Segher Boessenkool 2021-10-19 14:43 ` Martin Liška 2021-11-04 10:08 ` Martin Liška 2021-11-04 11:38 ` Segher Boessenkool 2021-10-19 14:11 ` Segher Boessenkool
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).