* [RFC/PATCH 00/11] Fix up some unexpected empty split conditions @ 2021-06-02 5:04 Kewen Lin 2021-06-02 5:04 ` [PATCH 01/11] gen: Emit error msg for empty split condition Kewen Lin ` (11 more replies) 0 siblings, 12 replies; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches Cc: Kewen Lin, wschmidt, segher, richard.sandiford, jeffreyalaw, jakub, richard.guenther Hi all, define_insn_and_split should avoid to use empty split condition if the condition for define_insn isn't empty, otherwise it can sometimes result in unexpected consequence, since the split will always be done even if the insn condition doesn't hold. To avoid forgetting to add "&& 1" onto split condition, as Segher suggested in thread[1], this series is to add the check and raise an error if it catches the unexpected cases. With this new check, we have to fix up some existing define_insn_and_split which are detected as error. I hope all these places are not intentional to be kept as blank. Any comments are highly appreciated. BR, Kewen [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566970.html Kewen Lin (11): gen: Emit error msg for empty split condition arc: Update unexpected empty split condition arm: Update unexpected empty split condition cris: Update unexpected empty split condition h8300: Update unexpected empty split condition i386: Update unexpected empty split condition m68k: Update unexpected empty split condition mips: Update unexpected empty split condition or1k: Update unexpected empty split condition sh: Update unexpected empty split condition sparc: Update unexpected empty split condition gcc/config/arc/arc.md | 2 +- gcc/config/arm/vfp.md | 4 ++-- gcc/config/cris/cris.md | 2 +- gcc/config/h8300/combiner.md | 2 +- gcc/config/i386/i386.md | 6 +++--- gcc/config/i386/sse.md | 8 ++++---- gcc/config/m68k/m68k.md | 6 +++--- gcc/config/mips/mips.md | 6 +++--- gcc/config/or1k/or1k.md | 2 +- gcc/config/sh/sh.md | 2 +- gcc/config/sparc/sparc.md | 12 ++++++------ gcc/gensupport.c | 3 +++ 12 files changed, 29 insertions(+), 26 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 01/11] gen: Emit error msg for empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 7:04 ` Richard Biener 2021-06-04 19:03 ` Martin Sebor 2021-06-02 5:04 ` [PATCH 02/11] arc: Update unexpected " Kewen Lin ` (10 subsequent siblings) 11 siblings, 2 replies; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches Cc: Kewen Lin, wschmidt, segher, richard.sandiford, jeffreyalaw, jakub, richard.guenther As Segher suggested, this patch is to emit the error message if the split condition of define_insn_and_split is empty while the insn condition isn't. gcc/ChangeLog: * gensupport.c (process_rtx): Emit error message for empty split condition in define_insn_and_split while the insn condition isn't. --- gcc/gensupport.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gcc/gensupport.c b/gcc/gensupport.c index 0f19bd70664..52cee120215 100644 --- a/gcc/gensupport.c +++ b/gcc/gensupport.c @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc) } else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) error_at (loc, "the rewrite condition must start with `&&'"); + else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0) + error_at (loc, "the split condition mustn't be empty if the " + "insn condition isn't empty"); XSTR (split, 1) = split_cond; if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 01/11] gen: Emit error msg for empty split condition 2021-06-02 5:04 ` [PATCH 01/11] gen: Emit error msg for empty split condition Kewen Lin @ 2021-06-02 7:04 ` Richard Biener 2021-06-02 7:27 ` Kewen.Lin 2021-06-04 19:03 ` Martin Sebor 1 sibling, 1 reply; 62+ messages in thread From: Richard Biener @ 2021-06-02 7:04 UTC (permalink / raw) To: Kewen Lin Cc: GCC Patches, Bill Schmidt, Segher Boessenkool, Richard Sandiford, Jeff Law, Jakub Jelinek On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote: > > As Segher suggested, this patch is to emit the error message > if the split condition of define_insn_and_split is empty while > the insn condition isn't. I wonder whether it would be a good idea to automagically make the split condition "&& 1" via gensupport? > gcc/ChangeLog: > > * gensupport.c (process_rtx): Emit error message for empty > split condition in define_insn_and_split while the insn > condition isn't. > --- > gcc/gensupport.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/gensupport.c b/gcc/gensupport.c > index 0f19bd70664..52cee120215 100644 > --- a/gcc/gensupport.c > +++ b/gcc/gensupport.c > @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc) > } > else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > error_at (loc, "the rewrite condition must start with `&&'"); > + else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0) > + error_at (loc, "the split condition mustn't be empty if the " > + "insn condition isn't empty"); > XSTR (split, 1) = split_cond; > if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 01/11] gen: Emit error msg for empty split condition 2021-06-02 7:04 ` Richard Biener @ 2021-06-02 7:27 ` Kewen.Lin 2021-06-02 7:43 ` Richard Biener 0 siblings, 1 reply; 62+ messages in thread From: Kewen.Lin @ 2021-06-02 7:27 UTC (permalink / raw) To: Richard Biener Cc: GCC Patches, Bill Schmidt, Segher Boessenkool, Richard Sandiford, Jeff Law, Jakub Jelinek Hi Richi, on 2021/6/2 下午3:04, Richard Biener wrote: > On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote: >> >> As Segher suggested, this patch is to emit the error message >> if the split condition of define_insn_and_split is empty while >> the insn condition isn't. > > I wonder whether it would be a good idea to automagically make > the split condition "&& 1" via gensupport? > Thanks for the comment! Do you happen to have some similar examples? IIUC this idea has to lay on the assumption always holding that when one developer puts split condition as blank meanwhile leaves insn condition as non-empty, he/she exactly wants to make split condition the same as insn condition. Once one developer doesn't think like that way (that is to want split to deal with more), the automatic condtion filling seems to over fill. The way that the current patch provided is not to allow the developer to write that kind of pattern, instead he/she has to go with separated define_insn and define_split. BR, Kewen >> gcc/ChangeLog: >> >> * gensupport.c (process_rtx): Emit error message for empty >> split condition in define_insn_and_split while the insn >> condition isn't. >> --- >> gcc/gensupport.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/gcc/gensupport.c b/gcc/gensupport.c >> index 0f19bd70664..52cee120215 100644 >> --- a/gcc/gensupport.c >> +++ b/gcc/gensupport.c >> @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc) >> } >> else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) >> error_at (loc, "the rewrite condition must start with `&&'"); >> + else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0) >> + error_at (loc, "the split condition mustn't be empty if the " >> + "insn condition isn't empty"); >> XSTR (split, 1) = split_cond; >> if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) >> XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 01/11] gen: Emit error msg for empty split condition 2021-06-02 7:27 ` Kewen.Lin @ 2021-06-02 7:43 ` Richard Biener 2021-06-02 8:18 ` Kewen.Lin 0 siblings, 1 reply; 62+ messages in thread From: Richard Biener @ 2021-06-02 7:43 UTC (permalink / raw) To: Kewen.Lin Cc: GCC Patches, Bill Schmidt, Segher Boessenkool, Richard Sandiford, Jeff Law, Jakub Jelinek On Wed, Jun 2, 2021 at 9:28 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > > Hi Richi, > > on 2021/6/2 下午3:04, Richard Biener wrote: > > On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote: > >> > >> As Segher suggested, this patch is to emit the error message > >> if the split condition of define_insn_and_split is empty while > >> the insn condition isn't. > > > > I wonder whether it would be a good idea to automagically make > > the split condition "&& 1" via gensupport? > > > > Thanks for the comment! Do you happen to have some similar examples? Not sure, the docs say @var{insn-pattern}, @var{condition}, @var{output-template}, and @var{insn-attributes} are used as in @code{define_insn}. ... The @var{split-condition} is also used as in @code{define_split}, with the additional behavior that if the condition starts with @samp{&&}, the condition used for the split will be the constructed as a logical ``and'' of the split condition with the insn condition. so one can indeed read this as "" meaning 'true' w/o considering the define_insn condition. But then we say The @code{define_insn_and_split} construction provides exactly the same functionality as two separate @code{define_insn} and @code{define_split} patterns. It exists for compactness, and as a maintenance tool to prevent having to ensure the two patterns' templates match. But then when I split a define_insn_and_split with a "" split condition they are not functionally identical? Also "" as split condition _does_ seem valid, just maybe unintended? How would one create a functionally equivalent example? "|| 1" will likely not work. Note I'm not familiar with all the details here but the documentation does seem ambiguous at best, not supporting to error on empty split-conditions at least. Richard. > IIUC this idea has to lay on the assumption always holding that when > one developer puts split condition as blank meanwhile leaves insn > condition as non-empty, he/she exactly wants to make split condition > the same as insn condition. Once one developer doesn't think like > that way (that is to want split to deal with more), the automatic > condtion filling seems to over fill. > > The way that the current patch provided is not to allow the developer > to write that kind of pattern, instead he/she has to go with separated > define_insn and define_split. > > BR, > Kewen > > >> gcc/ChangeLog: > >> > >> * gensupport.c (process_rtx): Emit error message for empty > >> split condition in define_insn_and_split while the insn > >> condition isn't. > >> --- > >> gcc/gensupport.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/gcc/gensupport.c b/gcc/gensupport.c > >> index 0f19bd70664..52cee120215 100644 > >> --- a/gcc/gensupport.c > >> +++ b/gcc/gensupport.c > >> @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc) > >> } > >> else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > >> error_at (loc, "the rewrite condition must start with `&&'"); > >> + else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0) > >> + error_at (loc, "the split condition mustn't be empty if the " > >> + "insn condition isn't empty"); > >> XSTR (split, 1) = split_cond; > >> if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > >> XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); > >> -- > >> 2.17.1 > >> > ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 01/11] gen: Emit error msg for empty split condition 2021-06-02 7:43 ` Richard Biener @ 2021-06-02 8:18 ` Kewen.Lin 2021-06-02 23:35 ` Segher Boessenkool 0 siblings, 1 reply; 62+ messages in thread From: Kewen.Lin @ 2021-06-02 8:18 UTC (permalink / raw) To: Richard Biener Cc: GCC Patches, Bill Schmidt, Segher Boessenkool, Richard Sandiford, Jeff Law, Jakub Jelinek on 2021/6/2 下午3:43, Richard Biener wrote: > On Wed, Jun 2, 2021 at 9:28 AM Kewen.Lin <linkw@linux.ibm.com> wrote: >> >> Hi Richi, >> >> on 2021/6/2 下午3:04, Richard Biener wrote: >>> On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote: >>>> >>>> As Segher suggested, this patch is to emit the error message >>>> if the split condition of define_insn_and_split is empty while >>>> the insn condition isn't. >>> >>> I wonder whether it would be a good idea to automagically make >>> the split condition "&& 1" via gensupport? >>> >> >> Thanks for the comment! Do you happen to have some similar examples? > > Not sure, the docs say > > @var{insn-pattern}, @var{condition}, @var{output-template}, and > @var{insn-attributes} are used as in @code{define_insn}. > ... > The @var{split-condition} is also used as in > @code{define_split}, with the additional behavior that if the condition starts > with @samp{&&}, the condition used for the split will be the constructed as a > logical ``and'' of the split condition with the insn condition. > > so one can indeed read this as "" meaning 'true' w/o considering the > define_insn condition. Yes, the "" in split condition does mean 'true' (always). > But then we say > > The @code{define_insn_and_split} construction provides exactly the same > functionality as two separate @code{define_insn} and @code{define_split} > patterns. It exists for compactness, and as a maintenance tool to prevent > having to ensure the two patterns' templates match. > > But then when I split a define_insn_and_split with a "" split condition > they are not functionally identical? Without this patch, they are indeed functionally identical. It's like the writer want to have one define_insn to match under some condition, but want to have one define_split to match always. > Also "" as split condition _does_ > seem valid, just maybe unintended? Yes, it's valid without this patch. That's why I asked whether there is some good reason to keep it be [1]. In Segher's opinion, there is no good reason, he pointed out "A reader does not expect a define_insn_and_split to split any other insns." > How would one create a > functionally equivalent example? "|| 1" will likely not work. > I think "|| 1" works just like "" if people want the define_split to split all the time, even with this patch. > Note I'm not familiar with all the details here but the documentation > does seem ambiguous at best, not supporting to error on empty > split-conditions at least. > Yes, the current patch will stop the "" condition which was accepted before. Thanks for bringing this up! We have to update the documentation if people reach a consensus. [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567014.html BR, Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 01/11] gen: Emit error msg for empty split condition 2021-06-02 8:18 ` Kewen.Lin @ 2021-06-02 23:35 ` Segher Boessenkool 0 siblings, 0 replies; 62+ messages in thread From: Segher Boessenkool @ 2021-06-02 23:35 UTC (permalink / raw) To: Kewen.Lin Cc: Richard Biener, GCC Patches, Bill Schmidt, Richard Sandiford, Jeff Law, Jakub Jelinek On Wed, Jun 02, 2021 at 04:18:46PM +0800, Kewen.Lin wrote: > on 2021/6/2 下午3:43, Richard Biener wrote: > Yes, the "" in split condition does mean 'true' (always). Right -- which means it will be split whenever it matches. This *can* be intended, but in define_insn_and_split it is almost always a simple bug. > > Also "" as split condition _does_ > > seem valid, just maybe unintended? > > Yes, it's valid without this patch. That's why I asked whether there is > some good reason to keep it be [1]. In Segher's opinion, there is no > good reason, he pointed out "A reader does not expect a > define_insn_and_split to split any other insns." Yes, but considering plain define_split, it can be wanted, esp. in simpler backends that do not have a lot of historical baggage. > > How would one create a > > functionally equivalent example? "|| 1" will likely not work. > > I think "|| 1" works just like "" if people want the define_split to > split all the time, even with this patch. Except "|| 1" is a syntax error. You can always write just "1". > > Note I'm not familiar with all the details here but the documentation > > does seem ambiguous at best, not supporting to error on empty > > split-conditions at least. > > Yes, the current patch will stop the "" condition which was accepted > before. Thanks for bringing this up! We have to update the > documentation if people reach a consensus. It will help if the error message tells you If this is what you intended, write "1". or similar. No more documentation is needed then :-) Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 01/11] gen: Emit error msg for empty split condition 2021-06-02 5:04 ` [PATCH 01/11] gen: Emit error msg for empty split condition Kewen Lin 2021-06-02 7:04 ` Richard Biener @ 2021-06-04 19:03 ` Martin Sebor 2021-06-04 19:37 ` Segher Boessenkool 1 sibling, 1 reply; 62+ messages in thread From: Martin Sebor @ 2021-06-04 19:03 UTC (permalink / raw) To: Kewen Lin, gcc-patches; +Cc: jakub, segher, richard.sandiford, wschmidt On 6/1/21 11:04 PM, Kewen Lin via Gcc-patches wrote: > As Segher suggested, this patch is to emit the error message > if the split condition of define_insn_and_split is empty while > the insn condition isn't. > > gcc/ChangeLog: > > * gensupport.c (process_rtx): Emit error message for empty > split condition in define_insn_and_split while the insn > condition isn't. > --- > gcc/gensupport.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/gensupport.c b/gcc/gensupport.c > index 0f19bd70664..52cee120215 100644 > --- a/gcc/gensupport.c > +++ b/gcc/gensupport.c > @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc) > } > else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > error_at (loc, "the rewrite condition must start with `&&'"); > + else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0) > + error_at (loc, "the split condition mustn't be empty if the " > + "insn condition isn't empty"); The "mustn't" (and other similar contractions) should trigger -Wdiag-format that GCC should be free of, or was not too long ago. Can you please spell them out (the suggested alternative spelling should be mentined in the warning)? Also, "insn" is not a word, and even though it's common abbreviation in GCC speak it's not necessarily something all users are familiar with, and doesn't lend itself to translation. Please spell out the word instead. Thanks Martin > XSTR (split, 1) = split_cond; > if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); > ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 01/11] gen: Emit error msg for empty split condition 2021-06-04 19:03 ` Martin Sebor @ 2021-06-04 19:37 ` Segher Boessenkool 0 siblings, 0 replies; 62+ messages in thread From: Segher Boessenkool @ 2021-06-04 19:37 UTC (permalink / raw) To: Martin Sebor; +Cc: Kewen Lin, gcc-patches, jakub, richard.sandiford, wschmidt On Fri, Jun 04, 2021 at 01:03:34PM -0600, Martin Sebor wrote: > Also, "insn" is not a word, and even though it's common abbreviation > in GCC speak it's not necessarily something all users are familiar > with, and doesn't lend itself to translation. Please spell out > the word instead. This is a message for GCC developers, and it is not translated. Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 02/11] arc: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin 2021-06-02 5:04 ` [PATCH 01/11] gen: Emit error msg for empty split condition Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 6:52 ` Claudiu Zissulescu 2021-06-02 5:04 ` [PATCH 03/11] arm: Update unexpected empty split condition Kewen Lin ` (9 subsequent siblings) 11 siblings, 1 reply; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches; +Cc: Kewen Lin, gnu, claziss, andrew.burgess gcc/ChangeLog: * config/arc/arc.md (*bbit_di): Fix empty split condition. --- gcc/config/arc/arc.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md index 7a52551eef5..a03840c4c36 100644 --- a/gcc/config/arc/arc.md +++ b/gcc/config/arc/arc.md @@ -5020,7 +5020,7 @@ (define_insn_and_split "*bbit_di" (clobber (reg:CC_ZN CC_REG))] "!CROSSING_JUMP_P (insn)" "#" - "" + "&& 1" [(parallel [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc))) (clobber (reg:CC_ZN CC_REG))])] -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 02/11] arc: Update unexpected empty split condition 2021-06-02 5:04 ` [PATCH 02/11] arc: Update unexpected " Kewen Lin @ 2021-06-02 6:52 ` Claudiu Zissulescu 2021-06-02 7:05 ` Kewen.Lin 0 siblings, 1 reply; 62+ messages in thread From: Claudiu Zissulescu @ 2021-06-02 6:52 UTC (permalink / raw) To: Kewen Lin, gcc-patches; +Cc: gnu, andrew.burgess Hi, Indeed, the split condition needs to be populated. However, I doubt that the pattern in question is used by the compiler. Do you have an example where it is exercised? Thanks, Claudiu ________________________________ From: Kewen Lin <linkw@linux.ibm.com> Sent: Wednesday, June 2, 2021 8:04 AM To: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org> Cc: Kewen Lin <linkw@linux.ibm.com>; gnu@amylaar.uk <gnu@amylaar.uk>; Claudiu Zissulescu <claziss@synopsys.com>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com> Subject: [PATCH 02/11] arc: Update unexpected empty split condition gcc/ChangeLog: * config/arc/arc.md (*bbit_di): Fix empty split condition. --- gcc/config/arc/arc.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md index 7a52551eef5..a03840c4c36 100644 --- a/gcc/config/arc/arc.md +++ b/gcc/config/arc/arc.md @@ -5020,7 +5020,7 @@ (define_insn_and_split "*bbit_di" (clobber (reg:CC_ZN CC_REG))] "!CROSSING_JUMP_P (insn)" "#" - "" + "&& 1" [(parallel [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc))) (clobber (reg:CC_ZN CC_REG))])] -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 02/11] arc: Update unexpected empty split condition 2021-06-02 6:52 ` Claudiu Zissulescu @ 2021-06-02 7:05 ` Kewen.Lin 2021-06-02 7:12 ` Claudiu Zissulescu 0 siblings, 1 reply; 62+ messages in thread From: Kewen.Lin @ 2021-06-02 7:05 UTC (permalink / raw) To: Claudiu Zissulescu, gcc-patches; +Cc: gnu, andrew.burgess Hi Claudiu, on 2021/6/2 下午2:52, Claudiu Zissulescu wrote: > Hi, > > Indeed, the split condition needs to be populated. However, I doubt that the pattern in question is used by the compiler. Do you have an example where it is exercised? > Thanks for the reply! Sorry that I don't have an example, the gensupport change will emit an error message for this pattern in build stage even without actual running. BR, Kewen > Thanks, > Claudiu > ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ > *From:* Kewen Lin <linkw@linux.ibm.com> > *Sent:* Wednesday, June 2, 2021 8:04 AM > *To:* gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org> > *Cc:* Kewen Lin <linkw@linux.ibm.com>; gnu@amylaar.uk <gnu@amylaar.uk>; Claudiu Zissulescu <claziss@synopsys.com>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com> > *Subject:* [PATCH 02/11] arc: Update unexpected empty split condition > > gcc/ChangeLog: > > * config/arc/arc.md (*bbit_di): Fix empty split condition. > --- > gcc/config/arc/arc.md | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md > index 7a52551eef5..a03840c4c36 100644 > --- a/gcc/config/arc/arc.md > +++ b/gcc/config/arc/arc.md > @@ -5020,7 +5020,7 @@ (define_insn_and_split "*bbit_di" > (clobber (reg:CC_ZN CC_REG))] > "!CROSSING_JUMP_P (insn)" > "#" > - "" > + "&& 1" > [(parallel > [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc))) > (clobber (reg:CC_ZN CC_REG))])] > -- > 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 02/11] arc: Update unexpected empty split condition 2021-06-02 7:05 ` Kewen.Lin @ 2021-06-02 7:12 ` Claudiu Zissulescu 2021-06-02 7:43 ` [PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di Kewen.Lin 0 siblings, 1 reply; 62+ messages in thread From: Claudiu Zissulescu @ 2021-06-02 7:12 UTC (permalink / raw) To: Kewen.Lin, gcc-patches; +Cc: gnu, andrew.burgess Hi Kewen, Maybe it is best just to remove the pattern entirely, I couldn't exercise it myself. I was secretly hopping someone could do it. Please can you submit a patch which removes it if it is not too much trouble? Thanks, Claudiu ________________________________ From: Kewen.Lin <linkw@linux.ibm.com> Sent: Wednesday, June 2, 2021 10:05 AM To: Claudiu Zissulescu <claziss@synopsys.com>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org> Cc: gnu@amylaar.uk <gnu@amylaar.uk>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com> Subject: Re: [PATCH 02/11] arc: Update unexpected empty split condition Hi Claudiu, on 2021/6/2 下午2:52, Claudiu Zissulescu wrote: > Hi, > > Indeed, the split condition needs to be populated. However, I doubt that the pattern in question is used by the compiler. Do you have an example where it is exercised? > Thanks for the reply! Sorry that I don't have an example, the gensupport change will emit an error message for this pattern in build stage even without actual running. BR, Kewen > Thanks, > Claudiu > ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ > *From:* Kewen Lin <linkw@linux.ibm.com> > *Sent:* Wednesday, June 2, 2021 8:04 AM > *To:* gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org> > *Cc:* Kewen Lin <linkw@linux.ibm.com>; gnu@amylaar.uk <gnu@amylaar.uk>; Claudiu Zissulescu <claziss@synopsys.com>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com> > *Subject:* [PATCH 02/11] arc: Update unexpected empty split condition > > gcc/ChangeLog: > > * config/arc/arc.md (*bbit_di): Fix empty split condition. > --- > gcc/config/arc/arc.md | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md > index 7a52551eef5..a03840c4c36 100644 > --- a/gcc/config/arc/arc.md > +++ b/gcc/config/arc/arc.md > @@ -5020,7 +5020,7 @@ (define_insn_and_split "*bbit_di" > (clobber (reg:CC_ZN CC_REG))] > "!CROSSING_JUMP_P (insn)" > "#" > - "" > + "&& 1" > [(parallel > [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc))) > (clobber (reg:CC_ZN CC_REG))])] > -- > 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di 2021-06-02 7:12 ` Claudiu Zissulescu @ 2021-06-02 7:43 ` Kewen.Lin 2021-06-02 8:33 ` Claudiu Zissulescu 0 siblings, 1 reply; 62+ messages in thread From: Kewen.Lin @ 2021-06-02 7:43 UTC (permalink / raw) To: Claudiu Zissulescu; +Cc: gnu, andrew.burgess, gcc-patches [-- Attachment #1: Type: text/plain, Size: 494 bytes --] Hi Claudiu, on 2021/6/2 下午3:12, Claudiu Zissulescu wrote: > Hi Kewen, > > Maybe it is best just to remove the pattern entirely, I couldn't exercise it myself. I was secretly hopping someone could do it. > Please can you submit a patch which removes it if it is not too much trouble? > The patch v2 has been attached which removes define_insn_and_split "*bbit_di" as you suggested. Does it look good to you? BR, Kewen ----- gcc/ChangeLog: * config/arc/arc.md (*bbit_di): Remove. [-- Attachment #2: arc_v2.diff --] [-- Type: text/plain, Size: 1419 bytes --] diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md index b6f2d8e28be..a67bb581003 100644 --- a/gcc/config/arc/arc.md +++ b/gcc/config/arc/arc.md @@ -5016,34 +5016,6 @@ (define_insn "*bbit" (if_then_else (match_test "get_attr_length (insn) == 6") (const_string "true") (const_string "false")))]) -; ??? When testing a bit from a DImode register, combine creates a -; zero_extract in DImode. This goes via an AND with a DImode constant, -; so can only be observed on 64 bit hosts. -(define_insn_and_split "*bbit_di" - [(set (pc) - (if_then_else - (match_operator 3 "equality_comparison_operator" - [(zero_extract:DI (match_operand:SI 1 "register_operand" "Rcqq,c") - (const_int 1) - (match_operand 2 "immediate_operand" "L,L")) - (const_int 0)]) - (label_ref (match_operand 0 "" "")) - (pc))) - (clobber (reg:CC_ZN CC_REG))] - "!CROSSING_JUMP_P (insn)" - "#" - "" - [(parallel - [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc))) - (clobber (reg:CC_ZN CC_REG))])] -{ - rtx xtr; - - xtr = gen_rtx_ZERO_EXTRACT (SImode, operands[1], const1_rtx, operands[2]); - operands[3] = gen_rtx_fmt_ee (GET_CODE (operands[3]), GET_MODE (operands[3]), - xtr, const0_rtx); -}) - ;; ------------------------------------------------------------------- ;; Hardware loop ;; ------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di 2021-06-02 7:43 ` [PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di Kewen.Lin @ 2021-06-02 8:33 ` Claudiu Zissulescu 0 siblings, 0 replies; 62+ messages in thread From: Claudiu Zissulescu @ 2021-06-02 8:33 UTC (permalink / raw) To: Kewen.Lin; +Cc: gnu, andrew.burgess, gcc-patches Looks good :) You can go ahead and commit it. Thank you for your contribution, Claudiu ________________________________ From: Kewen.Lin <linkw@linux.ibm.com> Sent: Wednesday, June 2, 2021 10:43 AM To: Claudiu Zissulescu <claziss@synopsys.com> Cc: gnu@amylaar.uk <gnu@amylaar.uk>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org> Subject: [PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di Hi Claudiu, on 2021/6/2 下午3:12, Claudiu Zissulescu wrote: > Hi Kewen, > > Maybe it is best just to remove the pattern entirely, I couldn't exercise it myself. I was secretly hopping someone could do it. > Please can you submit a patch which removes it if it is not too much trouble? > The patch v2 has been attached which removes define_insn_and_split "*bbit_di" as you suggested. Does it look good to you? BR, Kewen ----- gcc/ChangeLog: * config/arc/arc.md (*bbit_di): Remove. ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 03/11] arm: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin 2021-06-02 5:04 ` [PATCH 01/11] gen: Emit error msg for empty split condition Kewen Lin 2021-06-02 5:04 ` [PATCH 02/11] arc: Update unexpected " Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 9:02 ` Kyrylo Tkachov 2021-06-02 5:04 ` [PATCH 04/11] cris: " Kewen Lin ` (8 subsequent siblings) 11 siblings, 1 reply; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches Cc: Kewen Lin, nickc, richard.earnshaw, ramana.radhakrishnan, kyrylo.tkachov gcc/ChangeLog: * config/arm/vfp.md (no_literal_pool_df_immediate, no_literal_pool_sf_immediate): Fix empty split condition. --- gcc/config/arm/vfp.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md index f97af92716b..55b6c1ac585 100644 --- a/gcc/config/arm/vfp.md +++ b/gcc/config/arm/vfp.md @@ -2129,7 +2129,7 @@ (define_insn_and_split "no_literal_pool_df_immediate" && !arm_const_double_rtx (operands[1]) && !(TARGET_VFP_DOUBLE && vfp3_const_double_rtx (operands[1]))" "#" - "" + "&& 1" [(const_int 0)] { long buf[2]; @@ -2154,7 +2154,7 @@ (define_insn_and_split "no_literal_pool_sf_immediate" && TARGET_VFP_BASE && !vfp3_const_double_rtx (operands[1])" "#" - "" + "&& 1" [(const_int 0)] { long buf; -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH 03/11] arm: Update unexpected empty split condition 2021-06-02 5:04 ` [PATCH 03/11] arm: Update unexpected empty split condition Kewen Lin @ 2021-06-02 9:02 ` Kyrylo Tkachov 0 siblings, 0 replies; 62+ messages in thread From: Kyrylo Tkachov @ 2021-06-02 9:02 UTC (permalink / raw) To: Kewen Lin, gcc-patches; +Cc: nickc, Richard Earnshaw, Ramana Radhakrishnan > -----Original Message----- > From: Kewen Lin <linkw@linux.ibm.com> > Sent: 02 June 2021 06:05 > To: gcc-patches@gcc.gnu.org > Cc: Kewen Lin <linkw@linux.ibm.com>; nickc@redhat.com; Richard > Earnshaw <Richard.Earnshaw@arm.com>; Ramana Radhakrishnan > <Ramana.Radhakrishnan@arm.com>; Kyrylo Tkachov > <Kyrylo.Tkachov@arm.com> > Subject: [PATCH 03/11] arm: Update unexpected empty split condition > > gcc/ChangeLog: > > * config/arm/vfp.md (no_literal_pool_df_immediate, > no_literal_pool_sf_immediate): Fix empty split condition. Ok, thanks for doing this. Kyrill > --- > gcc/config/arm/vfp.md | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md > index f97af92716b..55b6c1ac585 100644 > --- a/gcc/config/arm/vfp.md > +++ b/gcc/config/arm/vfp.md > @@ -2129,7 +2129,7 @@ (define_insn_and_split > "no_literal_pool_df_immediate" > && !arm_const_double_rtx (operands[1]) > && !(TARGET_VFP_DOUBLE && vfp3_const_double_rtx (operands[1]))" > "#" > - "" > + "&& 1" > [(const_int 0)] > { > long buf[2]; > @@ -2154,7 +2154,7 @@ (define_insn_and_split > "no_literal_pool_sf_immediate" > && TARGET_VFP_BASE > && !vfp3_const_double_rtx (operands[1])" > "#" > - "" > + "&& 1" > [(const_int 0)] > { > long buf; > -- > 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 04/11] cris: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (2 preceding siblings ...) 2021-06-02 5:04 ` [PATCH 03/11] arm: Update unexpected empty split condition Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 12:45 ` Hans-Peter Nilsson 2021-06-02 5:04 ` [PATCH 05/11] h8300: " Kewen Lin ` (7 subsequent siblings) 11 siblings, 1 reply; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches; +Cc: Kewen Lin, hp gcc/ChangeLog: * config/cris/cris.md (*addi_reload): Fix empty split condition. --- gcc/config/cris/cris.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/config/cris/cris.md b/gcc/config/cris/cris.md index 7de0ec63fcf..d5a3c703a83 100644 --- a/gcc/config/cris/cris.md +++ b/gcc/config/cris/cris.md @@ -1311,7 +1311,7 @@ (define_insn_and_split "*addi_reload" && (INTVAL (operands[3]) == 2 || INTVAL (operands[3]) == 4) && (reload_in_progress || reload_completed)" "#" - "" + "&& 1" [(set (match_dup 0) (plus:SI (ashift:SI (match_dup 2) (match_dup 3)) (match_dup 1)))] "operands[3] = operands[3] == const2_rtx ? const1_rtx : const2_rtx;") -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 04/11] cris: Update unexpected empty split condition 2021-06-02 5:04 ` [PATCH 04/11] cris: " Kewen Lin @ 2021-06-02 12:45 ` Hans-Peter Nilsson 2021-06-03 5:45 ` Kewen.Lin 0 siblings, 1 reply; 62+ messages in thread From: Hans-Peter Nilsson @ 2021-06-02 12:45 UTC (permalink / raw) To: Kewen Lin; +Cc: gcc-patches > From: Kewen Lin <linkw@linux.ibm.com> > Date: Wed, 2 Jun 2021 07:04:54 +0200 > gcc/ChangeLog: > > * config/cris/cris.md (*addi_reload): Fix empty split condition. > --- > gcc/config/cris/cris.md | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/config/cris/cris.md b/gcc/config/cris/cris.md > index 7de0ec63fcf..d5a3c703a83 100644 > --- a/gcc/config/cris/cris.md > +++ b/gcc/config/cris/cris.md > @@ -1311,7 +1311,7 @@ (define_insn_and_split "*addi_reload" > && (INTVAL (operands[3]) == 2 || INTVAL (operands[3]) == 4) > && (reload_in_progress || reload_completed)" > "#" > - "" > + "&& 1" > [(set (match_dup 0) > (plus:SI (ashift:SI (match_dup 2) (match_dup 3)) (match_dup 1)))] > "operands[3] = operands[3] == const2_rtx ? const1_rtx : const2_rtx;") > -- > 2.17.1 > Ok, thanks, if only for all-round consistency. In preparation for a warning for an empty condition? I'm usually all for .md-warnings, but I'm not sure about the benefit of that one, though. Those "&& 1" look...hackish. brgds, H-P ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 04/11] cris: Update unexpected empty split condition 2021-06-02 12:45 ` Hans-Peter Nilsson @ 2021-06-03 5:45 ` Kewen.Lin 2021-06-03 16:12 ` Hans-Peter Nilsson 0 siblings, 1 reply; 62+ messages in thread From: Kewen.Lin @ 2021-06-03 5:45 UTC (permalink / raw) To: Hans-Peter Nilsson; +Cc: gcc-patches Hi Nilsson, on 2021/6/2 下午8:45, Hans-Peter Nilsson wrote: >> From: Kewen Lin <linkw@linux.ibm.com> >> Date: Wed, 2 Jun 2021 07:04:54 +0200 > >> gcc/ChangeLog: >> >> * config/cris/cris.md (*addi_reload): Fix empty split condition. >> --- >> gcc/config/cris/cris.md | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gcc/config/cris/cris.md b/gcc/config/cris/cris.md >> index 7de0ec63fcf..d5a3c703a83 100644 >> --- a/gcc/config/cris/cris.md >> +++ b/gcc/config/cris/cris.md >> @@ -1311,7 +1311,7 @@ (define_insn_and_split "*addi_reload" >> && (INTVAL (operands[3]) == 2 || INTVAL (operands[3]) == 4) >> && (reload_in_progress || reload_completed)" >> "#" >> - "" >> + "&& 1" >> [(set (match_dup 0) >> (plus:SI (ashift:SI (match_dup 2) (match_dup 3)) (match_dup 1)))] >> "operands[3] = operands[3] == const2_rtx ? const1_rtx : const2_rtx;") >> -- >> 2.17.1 >> > > Ok, thanks, if only for all-round consistency. > > In preparation for a warning for an empty condition? I'm > usually all for .md-warnings, but I'm not sure about the > benefit of that one, though. Those "&& 1" look...hackish. Thanks! Yeah, the 01/11 patch aims to raise one error message for the define_insn_and_split whose split condition is empty while insn condition isn't. In most cases, when we write one define_insn_and_split we want the splitting only to take effect while we see the define_insn matching happen (insn cond holds), but if we leave the split condition empty, the splitting will be done always, it could result in some unexpected consequence. Mostly this is unintentional. The error message is to avoid people to make it unintentionally. As you may have seen from the discussion under the 00/11 thread, we will probably end up with some other solution, so I will hold the changes for the ports, sorry for wasting your time and the other port maintainers'. BR, Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 04/11] cris: Update unexpected empty split condition 2021-06-03 5:45 ` Kewen.Lin @ 2021-06-03 16:12 ` Hans-Peter Nilsson 2021-06-03 22:33 ` Hans-Peter Nilsson 2021-06-04 3:25 ` Kewen.Lin 0 siblings, 2 replies; 62+ messages in thread From: Hans-Peter Nilsson @ 2021-06-03 16:12 UTC (permalink / raw) To: Kewen.Lin; +Cc: gcc-patches > From: Kewen.Lin <linkw@linux.ibm.com> > Date: Thu, 3 Jun 2021 07:45:57 +0200 > on 2021/6/2 Hans-Peter Nilsson wrote: > >> From: Kewen Lin <linkw@linux.ibm.com> > >> Date: Wed, 2 Jun 2021 07:04:54 +0200 > > > >> gcc/ChangeLog: > >> > >> * config/cris/cris.md (*addi_reload): Fix empty split condition. > >> - "" > >> + "&& 1" > > Ok, thanks, if only for all-round consistency. > > > > In preparation for a warning for an empty condition? I'm > > usually all for .md-warnings, but I'm not sure about the > > benefit of that one, though. Those "&& 1" look...hackish. > > Thanks! Yeah, the 01/11 patch aims to raise one error message > for the define_insn_and_split whose split condition is empty > while insn condition isn't. In most cases, when we write one > define_insn_and_split we want the splitting only to take effect > while we see the define_insn matching happen (insn cond holds), > but if we leave the split condition empty, the splitting will > be done always, it could result in some unexpected consequence. > Mostly this is unintentional. It certainly was in the patch above! > The error message is to avoid > people to make it unintentionally. > > As you may have seen from the discussion under the 00/11 thread, > we will probably end up with some other solution, so I will hold > the changes for the ports, sorry for wasting your time and the > other port maintainers'. No worries: I certainly don't consider it wasted and I'd prefer to have the patch above committed sooner than the conclusion of that discussion. (If you don't get to it, I'll do it, after a round of testing.) If you're considering further target patches to adjust for eventually changed semantics in the define_insn_and_split split-condition, then whatever trivial patch to cris.md that gets the effect of the one you sent is preapproved. Again, thanks. brgds, H-P ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 04/11] cris: Update unexpected empty split condition 2021-06-03 16:12 ` Hans-Peter Nilsson @ 2021-06-03 22:33 ` Hans-Peter Nilsson 2021-06-04 3:25 ` Kewen.Lin 1 sibling, 0 replies; 62+ messages in thread From: Hans-Peter Nilsson @ 2021-06-03 22:33 UTC (permalink / raw) To: linkw; +Cc: gcc-patches > From: Hans-Peter Nilsson <hp@axis.com> > CC: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org> > Date: Thu, 3 Jun 2021 18:12:25 +0200 > I'd > prefer to have the patch above committed sooner than the > conclusion of that discussion. (If you don't get to it, > I'll do it, after a round of testing.) Done; no regressions. brgds, H-P ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 04/11] cris: Update unexpected empty split condition 2021-06-03 16:12 ` Hans-Peter Nilsson 2021-06-03 22:33 ` Hans-Peter Nilsson @ 2021-06-04 3:25 ` Kewen.Lin 1 sibling, 0 replies; 62+ messages in thread From: Kewen.Lin @ 2021-06-04 3:25 UTC (permalink / raw) To: Hans-Peter Nilsson; +Cc: gcc-patches on 2021/6/4 上午12:12, Hans-Peter Nilsson wrote: >> From: Kewen.Lin <linkw@linux.ibm.com> >> Date: Thu, 3 Jun 2021 07:45:57 +0200 > >> on 2021/6/2 Hans-Peter Nilsson wrote: >>>> From: Kewen Lin <linkw@linux.ibm.com> >>>> Date: Wed, 2 Jun 2021 07:04:54 +0200 >>> >>>> gcc/ChangeLog: >>>> >>>> * config/cris/cris.md (*addi_reload): Fix empty split condition. > >>>> - "" >>>> + "&& 1" > >>> Ok, thanks, if only for all-round consistency. >>> >>> In preparation for a warning for an empty condition? I'm >>> usually all for .md-warnings, but I'm not sure about the >>> benefit of that one, though. Those "&& 1" look...hackish. >> >> Thanks! Yeah, the 01/11 patch aims to raise one error message >> for the define_insn_and_split whose split condition is empty >> while insn condition isn't. In most cases, when we write one >> define_insn_and_split we want the splitting only to take effect >> while we see the define_insn matching happen (insn cond holds), >> but if we leave the split condition empty, the splitting will >> be done always, it could result in some unexpected consequence. >> Mostly this is unintentional. > > It certainly was in the patch above! > >> The error message is to avoid >> people to make it unintentionally. >> >> As you may have seen from the discussion under the 00/11 thread, >> we will probably end up with some other solution, so I will hold >> the changes for the ports, sorry for wasting your time and the >> other port maintainers'. > > No worries: I certainly don't consider it wasted and I'd > prefer to have the patch above committed sooner than the > conclusion of that discussion. (If you don't get to it, > I'll do it, after a round of testing.) > Thanks for your help on testing! > If you're considering further target patches to adjust for > eventually changed semantics in the define_insn_and_split > split-condition, then whatever trivial patch to cris.md that > gets the effect of the one you sent is preapproved. > OK, thanks again! BR, Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 05/11] h8300: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (3 preceding siblings ...) 2021-06-02 5:04 ` [PATCH 04/11] cris: " Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 17:10 ` Jeff Law 2021-06-02 5:04 ` [PATCH 06/11] i386: " Kewen Lin ` (6 subsequent siblings) 11 siblings, 1 reply; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches gcc/ChangeLog: * config/h8300/combiner.md (*andsi3_lshiftrt_n_sb): Fix empty split condition. --- gcc/config/h8300/combiner.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/config/h8300/combiner.md b/gcc/config/h8300/combiner.md index 20e19da0419..e31bd507a6f 100644 --- a/gcc/config/h8300/combiner.md +++ b/gcc/config/h8300/combiner.md @@ -271,7 +271,7 @@ (define_insn_and_split "*andsi3_lshiftrt_n_sb" "exact_log2 (INTVAL (operands[3])) < 16 && INTVAL (operands[2]) + exact_log2 (INTVAL (operands[3])) == 31" "#" - "" + "&& 1" [(parallel [(set (match_dup 0) (and:SI (lshiftrt:SI (match_dup 1) (match_dup 2)) (match_dup 3))) -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 05/11] h8300: Update unexpected empty split condition 2021-06-02 5:04 ` [PATCH 05/11] h8300: " Kewen Lin @ 2021-06-02 17:10 ` Jeff Law 0 siblings, 0 replies; 62+ messages in thread From: Jeff Law @ 2021-06-02 17:10 UTC (permalink / raw) To: Kewen Lin, gcc-patches On 6/1/2021 11:04 PM, Kewen Lin wrote: > gcc/ChangeLog: > > * config/h8300/combiner.md (*andsi3_lshiftrt_n_sb): Fix empty split > condition. Hold off on this. We may need a stronger condition in there and that's something I'm in the process of cleaning up in the H8 port. jeff ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 06/11] i386: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (4 preceding siblings ...) 2021-06-02 5:04 ` [PATCH 05/11] h8300: " Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 6:28 ` Uros Bizjak 2021-06-02 5:04 ` [PATCH 07/11] m68k: " Kewen Lin ` (5 subsequent siblings) 11 siblings, 1 reply; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches; +Cc: Kewen Lin, hubicka, ubizjak gcc/ChangeLog: * config/i386/i386.md (*load_tp_x32_zext, *add_tp_x32_zext, *tls_dynamic_gnu2_combine_32): Fix empty split condition. * config/i386/sse.md (*<sse2_avx2>_pmovmskb_lt, *<sse2_avx2>_pmovmskb_zext_lt, *sse2_pmovmskb_ext_lt, *<sse4_1_avx2>_pblendvb_lt): Likewise. --- gcc/config/i386/i386.md | 6 +++--- gcc/config/i386/sse.md | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index 9ff35d9a607..545d048906d 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -15712,7 +15712,7 @@ (define_insn_and_split "*load_tp_x32_zext" (unspec:SI [(const_int 0)] UNSPEC_TP)))] "TARGET_X32" "#" - "" + "&& 1" [(set (match_dup 0) (zero_extend:DI (match_dup 1)))] { @@ -15750,7 +15750,7 @@ (define_insn_and_split "*add_tp_x32_zext" (clobber (reg:CC FLAGS_REG))] "TARGET_X32" "#" - "" + "&& 1" [(parallel [(set (match_dup 0) (zero_extend:DI @@ -15841,7 +15841,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_32" (clobber (reg:CC FLAGS_REG))] "!TARGET_64BIT && TARGET_GNU2_TLS" "#" - "" + "&& 1" [(set (match_dup 0) (match_dup 5))] { operands[5] = can_create_pseudo_p () ? gen_reg_rtx (Pmode) : operands[0]; diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md index 9d3728d1cb0..a9d78030119 100644 --- a/gcc/config/i386/sse.md +++ b/gcc/config/i386/sse.md @@ -16467,7 +16467,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_lt" UNSPEC_MOVMSK))] "TARGET_SSE2" "#" - "" + "&& 1" [(set (match_dup 0) (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))] "" @@ -16489,7 +16489,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_zext_lt" UNSPEC_MOVMSK)))] "TARGET_64BIT && TARGET_SSE2" "#" - "" + "&& 1" [(set (match_dup 0) (zero_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))] "" @@ -16511,7 +16511,7 @@ (define_insn_and_split "*sse2_pmovmskb_ext_lt" UNSPEC_MOVMSK)))] "TARGET_64BIT && TARGET_SSE2" "#" - "" + "&& 1" [(set (match_dup 0) (sign_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))] "" @@ -17769,7 +17769,7 @@ (define_insn_and_split "*<sse4_1_avx2>_pblendvb_lt" UNSPEC_BLENDV))] "TARGET_SSE4_1" "#" - "" + "&& 1" [(set (match_dup 0) (unspec:VI1_AVX2 [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPEC_BLENDV))] -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 06/11] i386: Update unexpected empty split condition 2021-06-02 5:04 ` [PATCH 06/11] i386: " Kewen Lin @ 2021-06-02 6:28 ` Uros Bizjak 0 siblings, 0 replies; 62+ messages in thread From: Uros Bizjak @ 2021-06-02 6:28 UTC (permalink / raw) To: Kewen Lin; +Cc: gcc-patches, Jan Hubicka On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote: > > gcc/ChangeLog: > > * config/i386/i386.md (*load_tp_x32_zext, *add_tp_x32_zext, > *tls_dynamic_gnu2_combine_32): Fix empty split condition. > * config/i386/sse.md (*<sse2_avx2>_pmovmskb_lt, > *<sse2_avx2>_pmovmskb_zext_lt, *sse2_pmovmskb_ext_lt, > *<sse4_1_avx2>_pblendvb_lt): Likewise. OK. Thanks, Uros. > --- > gcc/config/i386/i386.md | 6 +++--- > gcc/config/i386/sse.md | 8 ++++---- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md > index 9ff35d9a607..545d048906d 100644 > --- a/gcc/config/i386/i386.md > +++ b/gcc/config/i386/i386.md > @@ -15712,7 +15712,7 @@ (define_insn_and_split "*load_tp_x32_zext" > (unspec:SI [(const_int 0)] UNSPEC_TP)))] > "TARGET_X32" > "#" > - "" > + "&& 1" > [(set (match_dup 0) > (zero_extend:DI (match_dup 1)))] > { > @@ -15750,7 +15750,7 @@ (define_insn_and_split "*add_tp_x32_zext" > (clobber (reg:CC FLAGS_REG))] > "TARGET_X32" > "#" > - "" > + "&& 1" > [(parallel > [(set (match_dup 0) > (zero_extend:DI > @@ -15841,7 +15841,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_32" > (clobber (reg:CC FLAGS_REG))] > "!TARGET_64BIT && TARGET_GNU2_TLS" > "#" > - "" > + "&& 1" > [(set (match_dup 0) (match_dup 5))] > { > operands[5] = can_create_pseudo_p () ? gen_reg_rtx (Pmode) : operands[0]; > diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md > index 9d3728d1cb0..a9d78030119 100644 > --- a/gcc/config/i386/sse.md > +++ b/gcc/config/i386/sse.md > @@ -16467,7 +16467,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_lt" > UNSPEC_MOVMSK))] > "TARGET_SSE2" > "#" > - "" > + "&& 1" > [(set (match_dup 0) > (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))] > "" > @@ -16489,7 +16489,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_zext_lt" > UNSPEC_MOVMSK)))] > "TARGET_64BIT && TARGET_SSE2" > "#" > - "" > + "&& 1" > [(set (match_dup 0) > (zero_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))] > "" > @@ -16511,7 +16511,7 @@ (define_insn_and_split "*sse2_pmovmskb_ext_lt" > UNSPEC_MOVMSK)))] > "TARGET_64BIT && TARGET_SSE2" > "#" > - "" > + "&& 1" > [(set (match_dup 0) > (sign_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))] > "" > @@ -17769,7 +17769,7 @@ (define_insn_and_split "*<sse4_1_avx2>_pblendvb_lt" > UNSPEC_BLENDV))] > "TARGET_SSE4_1" > "#" > - "" > + "&& 1" > [(set (match_dup 0) > (unspec:VI1_AVX2 > [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPEC_BLENDV))] > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 07/11] m68k: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (5 preceding siblings ...) 2021-06-02 5:04 ` [PATCH 06/11] i386: " Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 17:08 ` Jeff Law 2021-06-02 5:04 ` [PATCH 08/11] mips: " Kewen Lin ` (4 subsequent siblings) 11 siblings, 1 reply; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches; +Cc: Kewen Lin, jeffreyalaw, schwab gcc/ChangeLog: * config/m68k/m68k.md (*zero_extend_inc, *zero_extend_dec, *zero_extendsidi2): Fix empty split condition. --- gcc/config/m68k/m68k.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gcc/config/m68k/m68k.md b/gcc/config/m68k/m68k.md index 59a456cd496..82d075e8bf0 100644 --- a/gcc/config/m68k/m68k.md +++ b/gcc/config/m68k/m68k.md @@ -1693,7 +1693,7 @@ (define_insn_and_split "*zero_extend_inc" GET_MODE_CLASS (GET_MODE (operands[1])) == MODE_INT && GET_MODE_SIZE (GET_MODE (operands[0])) == GET_MODE_SIZE (GET_MODE (operands[1])) * 2" "#" - "" + "&& 1" [(set (match_dup 0) (const_int 0)) (set (match_dup 0) @@ -1710,7 +1710,7 @@ (define_insn_and_split "*zero_extend_dec" GET_MODE_CLASS (GET_MODE (operands[1])) == MODE_INT && GET_MODE_SIZE (GET_MODE (operands[0])) == GET_MODE_SIZE (GET_MODE (operands[1])) * 2" "#" - "" + "&& 1" [(set (match_dup 0) (match_dup 1)) (set (match_dup 0) @@ -1764,7 +1764,7 @@ (define_insn_and_split "*zero_extendsidi2" (zero_extend:DI (match_operand:SI 1 "nonimmediate_src_operand" "")))] "GET_CODE (operands[0]) != MEM || GET_CODE (operands[1]) != MEM" "#" - "" + "&& 1" [(set (match_dup 2) (match_dup 1)) (set (match_dup 3) -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 07/11] m68k: Update unexpected empty split condition 2021-06-02 5:04 ` [PATCH 07/11] m68k: " Kewen Lin @ 2021-06-02 17:08 ` Jeff Law 0 siblings, 0 replies; 62+ messages in thread From: Jeff Law @ 2021-06-02 17:08 UTC (permalink / raw) To: Kewen Lin, gcc-patches; +Cc: schwab On 6/1/2021 11:04 PM, Kewen Lin wrote: > gcc/ChangeLog: > > * config/m68k/m68k.md (*zero_extend_inc, *zero_extend_dec, > *zero_extendsidi2): Fix empty split condition. OK. Thanks. jeff ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 08/11] mips: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (6 preceding siblings ...) 2021-06-02 5:04 ` [PATCH 07/11] m68k: " Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 17:11 ` Jeff Law 2021-06-02 5:04 ` [PATCH 09/11] or1k: " Kewen Lin ` (3 subsequent siblings) 11 siblings, 1 reply; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches; +Cc: Kewen Lin, mfortune gcc/ChangeLog: * config/mips/mips.md (<anonymous>, bswapsi2, bswapdi2): Fix empty split condition. --- gcc/config/mips/mips.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md index eef3cfd50a8..455b9b802f6 100644 --- a/gcc/config/mips/mips.md +++ b/gcc/config/mips/mips.md @@ -5835,7 +5835,7 @@ (define_insn_and_split "" (match_operand:SI 2 "immediate_operand" "I")))] "TARGET_MIPS16" "#" - "" + "&& 1" [(set (match_dup 0) (match_dup 1)) (set (match_dup 0) (lshiftrt:SI (match_dup 0) (match_dup 2)))] "" @@ -5871,7 +5871,7 @@ (define_insn_and_split "bswapsi2" (bswap:SI (match_operand:SI 1 "register_operand" "d")))] "ISA_HAS_WSBH && ISA_HAS_ROR" "#" - "" + "&& 1" [(set (match_dup 0) (unspec:SI [(match_dup 1)] UNSPEC_WSBH)) (set (match_dup 0) (rotatert:SI (match_dup 0) (const_int 16)))] "" @@ -5882,7 +5882,7 @@ (define_insn_and_split "bswapdi2" (bswap:DI (match_operand:DI 1 "register_operand" "d")))] "TARGET_64BIT && ISA_HAS_WSBH" "#" - "" + "&& 1" [(set (match_dup 0) (unspec:DI [(match_dup 1)] UNSPEC_DSBH)) (set (match_dup 0) (unspec:DI [(match_dup 0)] UNSPEC_DSHD))] "" -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 08/11] mips: Update unexpected empty split condition 2021-06-02 5:04 ` [PATCH 08/11] mips: " Kewen Lin @ 2021-06-02 17:11 ` Jeff Law 0 siblings, 0 replies; 62+ messages in thread From: Jeff Law @ 2021-06-02 17:11 UTC (permalink / raw) To: Kewen Lin, gcc-patches; +Cc: mfortune On 6/1/2021 11:04 PM, Kewen Lin via Gcc-patches wrote: > gcc/ChangeLog: > > * config/mips/mips.md (<anonymous>, bswapsi2, bswapdi2): Fix empty > split condition. The mips, or1k and sparc changes are fine. They're all preserving existing behavior. jeff ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 09/11] or1k: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (7 preceding siblings ...) 2021-06-02 5:04 ` [PATCH 08/11] mips: " Kewen Lin @ 2021-06-02 5:04 ` Kewen Lin 2021-06-02 5:05 ` [PATCH 10/11] sh: " Kewen Lin ` (2 subsequent siblings) 11 siblings, 0 replies; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:04 UTC (permalink / raw) To: gcc-patches gcc/ChangeLog: * config/or1k/or1k.md (*movdi): Fix empty split condition. --- gcc/config/or1k/or1k.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/config/or1k/or1k.md b/gcc/config/or1k/or1k.md index eb94efba0e4..495b3e277ba 100644 --- a/gcc/config/or1k/or1k.md +++ b/gcc/config/or1k/or1k.md @@ -351,7 +351,7 @@ (define_insn_and_split "*movdi" "register_operand (operands[0], DImode) || reg_or_0_operand (operands[1], DImode)" "#" - "" + "&& 1" [(const_int 0)] { rtx l0 = operand_subword (operands[0], 0, 0, DImode); -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 10/11] sh: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (8 preceding siblings ...) 2021-06-02 5:04 ` [PATCH 09/11] or1k: " Kewen Lin @ 2021-06-02 5:05 ` Kewen Lin 2021-06-02 5:17 ` Oleg Endo 2021-06-02 5:05 ` [PATCH 11/11] sparc: " Kewen Lin 2021-06-02 8:11 ` [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Richard Sandiford 11 siblings, 1 reply; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:05 UTC (permalink / raw) To: gcc-patches; +Cc: Kewen Lin, aoliva, olegendo gcc/ChangeLog: * config/sh/sh.md (doloop_end_split): Fix empty split condition. --- gcc/config/sh/sh.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md index e3af9ae21c1..93ee7c9a7de 100644 --- a/gcc/config/sh/sh.md +++ b/gcc/config/sh/sh.md @@ -6424,7 +6424,7 @@ (define_insn_and_split "doloop_end_split" (clobber (reg:SI T_REG))] "TARGET_SH2" "#" - "" + "&& 1" [(parallel [(set (reg:SI T_REG) (eq:SI (match_dup 2) (const_int 1))) (set (match_dup 0) (plus:SI (match_dup 2) (const_int -1)))]) -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 10/11] sh: Update unexpected empty split condition 2021-06-02 5:05 ` [PATCH 10/11] sh: " Kewen Lin @ 2021-06-02 5:17 ` Oleg Endo 0 siblings, 0 replies; 62+ messages in thread From: Oleg Endo @ 2021-06-02 5:17 UTC (permalink / raw) To: Kewen Lin, gcc-patches; +Cc: aoliva, olegendo On Wed, 2021-06-02 at 00:05 -0500, Kewen Lin wrote: > gcc/ChangeLog: > > * config/sh/sh.md (doloop_end_split): Fix empty split condition. > --- > gcc/config/sh/sh.md | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md > index e3af9ae21c1..93ee7c9a7de 100644 > --- a/gcc/config/sh/sh.md > +++ b/gcc/config/sh/sh.md > @@ -6424,7 +6424,7 @@ (define_insn_and_split "doloop_end_split" > (clobber (reg:SI T_REG))] > "TARGET_SH2" > "#" > - "" > + "&& 1" > [(parallel [(set (reg:SI T_REG) > (eq:SI (match_dup 2) (const_int 1))) > (set (match_dup 0) (plus:SI (match_dup 2) (const_int -1)))]) This is OK (obvious). Cheers, Oleg ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 11/11] sparc: Update unexpected empty split condition 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (9 preceding siblings ...) 2021-06-02 5:05 ` [PATCH 10/11] sh: " Kewen Lin @ 2021-06-02 5:05 ` Kewen Lin 2021-06-02 8:11 ` [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Richard Sandiford 11 siblings, 0 replies; 62+ messages in thread From: Kewen Lin @ 2021-06-02 5:05 UTC (permalink / raw) To: gcc-patches; +Cc: Kewen Lin, davem, ebotcazou gcc/ChangeLog: * config/sparc/sparc.md (*snedi<W:mode>_zero_vis3, *neg_snedi<W:mode>_zero_subxc, *plus_snedi<W:mode>_zero, *plus_plus_snedi<W:mode>_zero, *minus_snedi<W:mode>_zero, *minus_minus_snedi<W:mode>_zero): Fix empty split condition. --- gcc/config/sparc/sparc.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gcc/config/sparc/sparc.md b/gcc/config/sparc/sparc.md index c5d369626cc..0f85cb192c8 100644 --- a/gcc/config/sparc/sparc.md +++ b/gcc/config/sparc/sparc.md @@ -855,7 +855,7 @@ (define_insn_and_split "*snedi<W:mode>_zero_vis3" (clobber (reg:CCX CC_REG))] "TARGET_ARCH64 && TARGET_VIS3" "#" - "" + "&& 1" [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1))) (set (match_dup 0) (ltu:W (reg:CCXC CC_REG) (const_int 0)))] "" @@ -882,7 +882,7 @@ (define_insn_and_split "*neg_snedi<W:mode>_zero_subxc" (clobber (reg:CCX CC_REG))] "TARGET_ARCH64 && TARGET_SUBXC" "#" - "" + "&& 1" [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1))) (set (match_dup 0) (neg:W (ltu:W (reg:CCXC CC_REG) (const_int 0))))] "" @@ -984,7 +984,7 @@ (define_insn_and_split "*plus_snedi<W:mode>_zero" (clobber (reg:CCX CC_REG))] "TARGET_ARCH64 && TARGET_VIS3" "#" - "" + "&& 1" [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1))) (set (match_dup 0) (plus:W (ltu:W (reg:CCXC CC_REG) (const_int 0)) (match_dup 2)))] @@ -1000,7 +1000,7 @@ (define_insn_and_split "*plus_plus_snedi<W:mode>_zero" (clobber (reg:CCX CC_REG))] "TARGET_ARCH64 && TARGET_VIS3" "#" - "" + "&& 1" [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1))) (set (match_dup 0) (plus:W (plus:W (ltu:W (reg:CCXC CC_REG) (const_int 0)) (match_dup 2)) @@ -1048,7 +1048,7 @@ (define_insn_and_split "*minus_snedi<W:mode>_zero" (clobber (reg:CCX CC_REG))] "TARGET_ARCH64 && TARGET_SUBXC" "#" - "" + "&& 1" [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1))) (set (match_dup 0) (minus:W (match_dup 2) (ltu:W (reg:CCXC CC_REG) (const_int 0))))] @@ -1064,7 +1064,7 @@ (define_insn_and_split "*minus_minus_snedi<W:mode>_zero" (clobber (reg:CCX CC_REG))] "TARGET_ARCH64 && TARGET_SUBXC" "#" - "" + "&& 1" [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1))) (set (match_dup 0) (minus:W (minus:W (match_dup 2) (ltu:W (reg:CCXC CC_REG) (const_int 0))) -- 2.17.1 ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin ` (10 preceding siblings ...) 2021-06-02 5:05 ` [PATCH 11/11] sparc: " Kewen Lin @ 2021-06-02 8:11 ` Richard Sandiford 2021-06-02 8:37 ` Kewen.Lin 11 siblings, 1 reply; 62+ messages in thread From: Richard Sandiford @ 2021-06-02 8:11 UTC (permalink / raw) To: Kewen Lin Cc: gcc-patches, wschmidt, segher, jeffreyalaw, jakub, richard.guenther Kewen Lin <linkw@linux.ibm.com> writes: > Hi all, > > define_insn_and_split should avoid to use empty split condition > if the condition for define_insn isn't empty, otherwise it can > sometimes result in unexpected consequence, since the split > will always be done even if the insn condition doesn't hold. > > To avoid forgetting to add "&& 1" onto split condition, as > Segher suggested in thread[1], this series is to add the check > and raise an error if it catches the unexpected cases. With > this new check, we have to fix up some existing > define_insn_and_split which are detected as error. I hope all > these places are not intentional to be kept as blank. I wonder whether we should instead redefine the semantics of define_insn_and_split so that the split condition is always applied on top of the insn condition. It's rare for a define_insn_and_split to have independent insn and split conditions, so at the moment, we're making the common case hard. Thanks, Richard > Any comments are highly appreciated. > > BR, > Kewen > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566970.html > > Kewen Lin (11): > gen: Emit error msg for empty split condition > arc: Update unexpected empty split condition > arm: Update unexpected empty split condition > cris: Update unexpected empty split condition > h8300: Update unexpected empty split condition > i386: Update unexpected empty split condition > m68k: Update unexpected empty split condition > mips: Update unexpected empty split condition > or1k: Update unexpected empty split condition > sh: Update unexpected empty split condition > sparc: Update unexpected empty split condition > > gcc/config/arc/arc.md | 2 +- > gcc/config/arm/vfp.md | 4 ++-- > gcc/config/cris/cris.md | 2 +- > gcc/config/h8300/combiner.md | 2 +- > gcc/config/i386/i386.md | 6 +++--- > gcc/config/i386/sse.md | 8 ++++---- > gcc/config/m68k/m68k.md | 6 +++--- > gcc/config/mips/mips.md | 6 +++--- > gcc/config/or1k/or1k.md | 2 +- > gcc/config/sh/sh.md | 2 +- > gcc/config/sparc/sparc.md | 12 ++++++------ > gcc/gensupport.c | 3 +++ > 12 files changed, 29 insertions(+), 26 deletions(-) ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 8:11 ` [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Richard Sandiford @ 2021-06-02 8:37 ` Kewen.Lin 2021-06-02 9:13 ` Richard Sandiford 0 siblings, 1 reply; 62+ messages in thread From: Kewen.Lin @ 2021-06-02 8:37 UTC (permalink / raw) To: richard.sandiford Cc: gcc-patches, wschmidt, segher, jeffreyalaw, jakub, richard.guenther Hi Richard, on 2021/6/2 下午4:11, Richard Sandiford wrote: > Kewen Lin <linkw@linux.ibm.com> writes: >> Hi all, >> >> define_insn_and_split should avoid to use empty split condition >> if the condition for define_insn isn't empty, otherwise it can >> sometimes result in unexpected consequence, since the split >> will always be done even if the insn condition doesn't hold. >> >> To avoid forgetting to add "&& 1" onto split condition, as >> Segher suggested in thread[1], this series is to add the check >> and raise an error if it catches the unexpected cases. With >> this new check, we have to fix up some existing >> define_insn_and_split which are detected as error. I hope all >> these places are not intentional to be kept as blank. > > I wonder whether we should instead redefine the semantics of > define_insn_and_split so that the split condition is always applied > on top of the insn condition. It's rare for a define_insn_and_split > to have independent insn and split conditions, so at the moment, > we're making the common case hard. > Just want to confirm that the suggestion is just applied for empty split condition or all split conditions in define_insn_and_split? I guess it's the former? It's like what Richi suggested with auto-filling, the semantics redefining will eliminate the over-fill concern. Thanks for the suggestion! BR, Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 8:37 ` Kewen.Lin @ 2021-06-02 9:13 ` Richard Sandiford 2021-06-02 10:01 ` Kewen.Lin 0 siblings, 1 reply; 62+ messages in thread From: Richard Sandiford @ 2021-06-02 9:13 UTC (permalink / raw) To: Kewen.Lin Cc: gcc-patches, wschmidt, segher, jeffreyalaw, jakub, richard.guenther "Kewen.Lin" <linkw@linux.ibm.com> writes: > Hi Richard, > > on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote: >> Kewen Lin <linkw@linux.ibm.com> writes: >>> Hi all, >>> >>> define_insn_and_split should avoid to use empty split condition >>> if the condition for define_insn isn't empty, otherwise it can >>> sometimes result in unexpected consequence, since the split >>> will always be done even if the insn condition doesn't hold. >>> >>> To avoid forgetting to add "&& 1" onto split condition, as >>> Segher suggested in thread[1], this series is to add the check >>> and raise an error if it catches the unexpected cases. With >>> this new check, we have to fix up some existing >>> define_insn_and_split which are detected as error. I hope all >>> these places are not intentional to be kept as blank. >> >> I wonder whether we should instead redefine the semantics of >> define_insn_and_split so that the split condition is always applied >> on top of the insn condition. It's rare for a define_insn_and_split >> to have independent insn and split conditions, so at the moment, >> we're making the common case hard. >> > > Just want to confirm that the suggestion is just applied for empty > split condition or all split conditions in define_insn_and_split? > I guess it's the former? No, I meant tha latter. E.g. in: (define_insn_and_split […] "TARGET_FOO" "…" […] "reload_completed" […] ) the "reload_completed" condition is almost always a typo for "&& reload_completed". Like I say, it rarely makes sense for the split condition to ignore the insn condition and specify an entirely independent condition. There might be some define_insn_and_splits that do that, but it'd often be less confusing to write the insn and split separately if so. Even if we do want to support independent insn and split conditions, that's always going to be the rare and surprising case, so it's the one that should need extra syntax. Thanks, Richard It's like what Richi suggested with > auto-filling, the semantics redefining will eliminate the over-fill > concern. Thanks for the suggestion! > > BR, > Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 9:13 ` Richard Sandiford @ 2021-06-02 10:01 ` Kewen.Lin 2021-06-02 10:12 ` Richard Biener 0 siblings, 1 reply; 62+ messages in thread From: Kewen.Lin @ 2021-06-02 10:01 UTC (permalink / raw) To: richard.sandiford Cc: gcc-patches, wschmidt, segher, jeffreyalaw, jakub, richard.guenther on 2021/6/2 下午5:13, Richard Sandiford wrote: > "Kewen.Lin" <linkw@linux.ibm.com> writes: >> Hi Richard, >> >> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote: >>> Kewen Lin <linkw@linux.ibm.com> writes: >>>> Hi all, >>>> >>>> define_insn_and_split should avoid to use empty split condition >>>> if the condition for define_insn isn't empty, otherwise it can >>>> sometimes result in unexpected consequence, since the split >>>> will always be done even if the insn condition doesn't hold. >>>> >>>> To avoid forgetting to add "&& 1" onto split condition, as >>>> Segher suggested in thread[1], this series is to add the check >>>> and raise an error if it catches the unexpected cases. With >>>> this new check, we have to fix up some existing >>>> define_insn_and_split which are detected as error. I hope all >>>> these places are not intentional to be kept as blank. >>> >>> I wonder whether we should instead redefine the semantics of >>> define_insn_and_split so that the split condition is always applied >>> on top of the insn condition. It's rare for a define_insn_and_split >>> to have independent insn and split conditions, so at the moment, >>> we're making the common case hard. >>> >> >> Just want to confirm that the suggestion is just applied for empty >> split condition or all split conditions in define_insn_and_split? >> I guess it's the former? > > No, I meant tha latter. E.g. in: > > (define_insn_and_split > […] > "TARGET_FOO" > "…" > […] > "reload_completed" > […] > ) > > the "reload_completed" condition is almost always a typo for > "&& reload_completed". > > Like I say, it rarely makes sense for the split condition to > ignore the insn condition and specify an entirely independent condition. > There might be some define_insn_and_splits that do that, but it'd often > be less confusing to write the insn and split separately if so. > > Even if we do want to support independent insn and split conditions, > that's always going to be the rare and surprising case, so it's the one > that should need extra syntax. > Thanks for the clarification! Since it may impact all ports, I wonder if there is a way to find out this kind of "rare and surprising" case without a big coverage testing? I'm happy to make a draft patch for it, but not sure how to early catch those cases which need to be rewritten for those ports that I can't test on (even with cfarm machines, the coverage seems still limited). BR, Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 10:01 ` Kewen.Lin @ 2021-06-02 10:12 ` Richard Biener 2021-06-02 17:32 ` Richard Sandiford 0 siblings, 1 reply; 62+ messages in thread From: Richard Biener @ 2021-06-02 10:12 UTC (permalink / raw) To: Kewen.Lin Cc: Richard Sandiford, GCC Patches, Bill Schmidt, Segher Boessenkool, Jeff Law, Jakub Jelinek On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote: > > on 2021/6/2 下午5:13, Richard Sandiford wrote: > > "Kewen.Lin" <linkw@linux.ibm.com> writes: > >> Hi Richard, > >> > >> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote: > >>> Kewen Lin <linkw@linux.ibm.com> writes: > >>>> Hi all, > >>>> > >>>> define_insn_and_split should avoid to use empty split condition > >>>> if the condition for define_insn isn't empty, otherwise it can > >>>> sometimes result in unexpected consequence, since the split > >>>> will always be done even if the insn condition doesn't hold. > >>>> > >>>> To avoid forgetting to add "&& 1" onto split condition, as > >>>> Segher suggested in thread[1], this series is to add the check > >>>> and raise an error if it catches the unexpected cases. With > >>>> this new check, we have to fix up some existing > >>>> define_insn_and_split which are detected as error. I hope all > >>>> these places are not intentional to be kept as blank. > >>> > >>> I wonder whether we should instead redefine the semantics of > >>> define_insn_and_split so that the split condition is always applied > >>> on top of the insn condition. It's rare for a define_insn_and_split > >>> to have independent insn and split conditions, so at the moment, > >>> we're making the common case hard. > >>> > >> > >> Just want to confirm that the suggestion is just applied for empty > >> split condition or all split conditions in define_insn_and_split? > >> I guess it's the former? > > > > No, I meant tha latter. E.g. in: > > > > (define_insn_and_split > > […] > > "TARGET_FOO" > > "…" > > […] > > "reload_completed" > > […] > > ) > > > > the "reload_completed" condition is almost always a typo for > > "&& reload_completed". > > > > Like I say, it rarely makes sense for the split condition to > > ignore the insn condition and specify an entirely independent condition. > > There might be some define_insn_and_splits that do that, but it'd often > > be less confusing to write the insn and split separately if so. > > > > Even if we do want to support independent insn and split conditions, > > that's always going to be the rare and surprising case, so it's the one > > that should need extra syntax. > > > > Thanks for the clarification! > > Since it may impact all ports, I wonder if there is a way to find out > this kind of "rare and surprising" case without a big coverage testing? > I'm happy to make a draft patch for it, but not sure how to early catch > those cases which need to be rewritten for those ports that I can't test > on (even with cfarm machines, the coverage seems still limited). So what Richard suggests would be to disallow split conditions that do not start with "&& ", it's probably easy to do that as well and look for build fails. That should catch all cases to look at. Richard. > BR, > Kewen > ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 10:12 ` Richard Biener @ 2021-06-02 17:32 ` Richard Sandiford 2021-06-02 18:25 ` Jeff Law 2021-06-02 23:52 ` Segher Boessenkool 0 siblings, 2 replies; 62+ messages in thread From: Richard Sandiford @ 2021-06-02 17:32 UTC (permalink / raw) To: Richard Biener Cc: Kewen.Lin, GCC Patches, Bill Schmidt, Segher Boessenkool, Jeff Law, Jakub Jelinek Richard Biener <richard.guenther@gmail.com> writes: > On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote: >> >> on 2021/6/2 下午5:13, Richard Sandiford wrote: >> > "Kewen.Lin" <linkw@linux.ibm.com> writes: >> >> Hi Richard, >> >> >> >> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote: >> >>> Kewen Lin <linkw@linux.ibm.com> writes: >> >>>> Hi all, >> >>>> >> >>>> define_insn_and_split should avoid to use empty split condition >> >>>> if the condition for define_insn isn't empty, otherwise it can >> >>>> sometimes result in unexpected consequence, since the split >> >>>> will always be done even if the insn condition doesn't hold. >> >>>> >> >>>> To avoid forgetting to add "&& 1" onto split condition, as >> >>>> Segher suggested in thread[1], this series is to add the check >> >>>> and raise an error if it catches the unexpected cases. With >> >>>> this new check, we have to fix up some existing >> >>>> define_insn_and_split which are detected as error. I hope all >> >>>> these places are not intentional to be kept as blank. >> >>> >> >>> I wonder whether we should instead redefine the semantics of >> >>> define_insn_and_split so that the split condition is always applied >> >>> on top of the insn condition. It's rare for a define_insn_and_split >> >>> to have independent insn and split conditions, so at the moment, >> >>> we're making the common case hard. >> >>> >> >> >> >> Just want to confirm that the suggestion is just applied for empty >> >> split condition or all split conditions in define_insn_and_split? >> >> I guess it's the former? >> > >> > No, I meant tha latter. E.g. in: >> > >> > (define_insn_and_split >> > […] >> > "TARGET_FOO" >> > "…" >> > […] >> > "reload_completed" >> > […] >> > ) >> > >> > the "reload_completed" condition is almost always a typo for >> > "&& reload_completed". >> > >> > Like I say, it rarely makes sense for the split condition to >> > ignore the insn condition and specify an entirely independent condition. >> > There might be some define_insn_and_splits that do that, but it'd often >> > be less confusing to write the insn and split separately if so. >> > >> > Even if we do want to support independent insn and split conditions, >> > that's always going to be the rare and surprising case, so it's the one >> > that should need extra syntax. >> > >> >> Thanks for the clarification! >> >> Since it may impact all ports, I wonder if there is a way to find out >> this kind of "rare and surprising" case without a big coverage testing? >> I'm happy to make a draft patch for it, but not sure how to early catch >> those cases which need to be rewritten for those ports that I can't test >> on (even with cfarm machines, the coverage seems still limited). > > So what Richard suggests would be to disallow split conditions > that do not start with "&& ", it's probably easy to do that as well > and look for build fails. That should catch all cases to look at. Yeah. As a strawman proposal, how about: - add a new "define_independent_insn_and_split" that has the current semantics of define_insn_and_split. This should be mechanical. - find the define_insn_and_splits that are missing the "&&", and where missing the "&&" might make a difference. Change them to define_independent_insn_and_splits. Like Richard says, this can be done by temporarily disallowing define_insn_and_splits that have no "&&". I think this should remain a mechanical step. If port maintainers think that the missing "&&" is a mistake, they should fix it as a separate patch. - flip the default for define_insn_and_split so that the "&&" is implicit (but can still be specified redundantly) Then port maintainers who don't mind the churn can remove the redundant "&&"s from the remaining define_insn_and_splits. Thanks, Richard ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 17:32 ` Richard Sandiford @ 2021-06-02 18:25 ` Jeff Law 2021-06-02 23:52 ` Segher Boessenkool 1 sibling, 0 replies; 62+ messages in thread From: Jeff Law @ 2021-06-02 18:25 UTC (permalink / raw) To: Richard Biener, Kewen.Lin, GCC Patches, Bill Schmidt, Segher Boessenkool, Jakub Jelinek, richard.sandiford On 6/2/2021 11:32 AM, Richard Sandiford wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote: >>> on 2021/6/2 下午5:13, Richard Sandiford wrote: >>>> "Kewen.Lin" <linkw@linux.ibm.com> writes: >>>>> Hi Richard, >>>>> >>>>> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote: >>>>>> Kewen Lin <linkw@linux.ibm.com> writes: >>>>>>> Hi all, >>>>>>> >>>>>>> define_insn_and_split should avoid to use empty split condition >>>>>>> if the condition for define_insn isn't empty, otherwise it can >>>>>>> sometimes result in unexpected consequence, since the split >>>>>>> will always be done even if the insn condition doesn't hold. >>>>>>> >>>>>>> To avoid forgetting to add "&& 1" onto split condition, as >>>>>>> Segher suggested in thread[1], this series is to add the check >>>>>>> and raise an error if it catches the unexpected cases. With >>>>>>> this new check, we have to fix up some existing >>>>>>> define_insn_and_split which are detected as error. I hope all >>>>>>> these places are not intentional to be kept as blank. >>>>>> I wonder whether we should instead redefine the semantics of >>>>>> define_insn_and_split so that the split condition is always applied >>>>>> on top of the insn condition. It's rare for a define_insn_and_split >>>>>> to have independent insn and split conditions, so at the moment, >>>>>> we're making the common case hard. >>>>>> >>>>> Just want to confirm that the suggestion is just applied for empty >>>>> split condition or all split conditions in define_insn_and_split? >>>>> I guess it's the former? >>>> No, I meant tha latter. E.g. in: >>>> >>>> (define_insn_and_split >>>> […] >>>> "TARGET_FOO" >>>> "…" >>>> […] >>>> "reload_completed" >>>> […] >>>> ) >>>> >>>> the "reload_completed" condition is almost always a typo for >>>> "&& reload_completed". >>>> >>>> Like I say, it rarely makes sense for the split condition to >>>> ignore the insn condition and specify an entirely independent condition. >>>> There might be some define_insn_and_splits that do that, but it'd often >>>> be less confusing to write the insn and split separately if so. >>>> >>>> Even if we do want to support independent insn and split conditions, >>>> that's always going to be the rare and surprising case, so it's the one >>>> that should need extra syntax. >>>> >>> Thanks for the clarification! >>> >>> Since it may impact all ports, I wonder if there is a way to find out >>> this kind of "rare and surprising" case without a big coverage testing? >>> I'm happy to make a draft patch for it, but not sure how to early catch >>> those cases which need to be rewritten for those ports that I can't test >>> on (even with cfarm machines, the coverage seems still limited). >> So what Richard suggests would be to disallow split conditions >> that do not start with "&& ", it's probably easy to do that as well >> and look for build fails. That should catch all cases to look at. > Yeah. As a strawman proposal, how about: > > - add a new "define_independent_insn_and_split" that has the > current semantics of define_insn_and_split. This should be > mechanical. > > - find the define_insn_and_splits that are missing the "&&", and where > missing the "&&" might make a difference. Change them to > define_independent_insn_and_splits. > > Like Richard says, this can be done by temporarily disallowing > define_insn_and_splits that have no "&&". > > I think this should remain a mechanical step. If port maintainers > think that the missing "&&" is a mistake, they should fix it as > a separate patch. > > - flip the default for define_insn_and_split so that the "&&" is implicit > (but can still be specified redundantly) > > Then port maintainers who don't mind the churn can remove the > redundant "&&"s from the remaining define_insn_and_splits. That works for me. If we'd had this in place earlier I wouldn't have mucked up the H8 port. jeff ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 17:32 ` Richard Sandiford 2021-06-02 18:25 ` Jeff Law @ 2021-06-02 23:52 ` Segher Boessenkool 2021-06-03 5:22 ` Kewen.Lin 1 sibling, 1 reply; 62+ messages in thread From: Segher Boessenkool @ 2021-06-02 23:52 UTC (permalink / raw) To: Richard Biener, Kewen.Lin, GCC Patches, Bill Schmidt, Jeff Law, Jakub Jelinek, richard.sandiford On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote: > Richard Biener <richard.guenther@gmail.com> writes: > > So what Richard suggests would be to disallow split conditions > > that do not start with "&& ", it's probably easy to do that as well > > and look for build fails. That should catch all cases to look at. > > Yeah. As a strawman proposal, how about: > > - add a new "define_independent_insn_and_split" that has the > current semantics of define_insn_and_split. This should be > mechanical. I'd rather not have that -- we can just write separate define_insn and define_split in that case. How many such cases *are* there? There are no users exposed to this, and when the split condition is required to start with "&&" (instead of getting that implied) it is not a silent change ever, either. > - find the define_insn_and_splits that are missing the "&&", and where > missing the "&&" might make a difference. Change them to > define_independent_insn_and_splits. > > Like Richard says, this can be done by temporarily disallowing > define_insn_and_splits that have no "&&". If we make that change permanently, that is all steps we ever need! Very old backends use the same insn condition and split condition sometimes still; it isn't hard to detect that as well, if that seems prudent. Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-02 23:52 ` Segher Boessenkool @ 2021-06-03 5:22 ` Kewen.Lin 2021-06-03 8:00 ` Segher Boessenkool 2021-06-03 8:05 ` Richard Sandiford 0 siblings, 2 replies; 62+ messages in thread From: Kewen.Lin @ 2021-06-03 5:22 UTC (permalink / raw) To: Segher Boessenkool, Richard Biener, Jeff Law, richard.sandiford Cc: GCC Patches, Bill Schmidt, Jakub Jelinek Hi Richi/Richard/Jeff/Segher, Thanks for the comments! on 2021/6/3 上午7:52, Segher Boessenkool wrote: > On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote: >> Richard Biener <richard.guenther@gmail.com> writes: >>> So what Richard suggests would be to disallow split conditions >>> that do not start with "&& ", it's probably easy to do that as well >>> and look for build fails. That should catch all cases to look at. >> >> Yeah. As a strawman proposal, how about: >> >> - add a new "define_independent_insn_and_split" that has the >> current semantics of define_insn_and_split. This should be >> mechanical. > > I'd rather not have that -- we can just write separate define_insn and > define_split in that case. > Not sure if someone would argue that he/she would like to go with one shared pattern as before, to avoid any possible differences between two seperated patterns and have good maintainability (like only editing on place) and slightly better efficiency. > How many such cases *are* there? There are no users exposed to this, > and when the split condition is required to start with "&&" (instead of > getting that implied) it is not a silent change ever, either. > If I read the proposal right, the explicit "&&" is only required when going to find all potential problematic places for final implied "&&" change. But one explicit "&&" does offer good readability. >> - find the define_insn_and_splits that are missing the "&&", and where >> missing the "&&" might make a difference. Change them to >> define_independent_insn_and_splits. >> >> Like Richard says, this can be done by temporarily disallowing >> define_insn_and_splits that have no "&&". > > If we make that change permanently, that is all steps we ever need! > So the question is that: whether we need to demand an explicit "&&". Richard's proposal is for answer "no" which aligns with Richi's auto filling advice before. I think it would result in fewer changes since those places without explicit "&&" are mostly unintentional, all the jobs are done by implied "&&". Its downside seems to be bad readability, new readers may take it as two seperated conditions at first glance, but I guess if we emphasize this change in the document it would be fine? Or emitting one warning if missing an explicit "&&"? BR, Kewen > Very old backends use the same insn condition and split condition > sometimes still; it isn't hard to detect that as well, if that seems > prudent. > > > Segher > ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 5:22 ` Kewen.Lin @ 2021-06-03 8:00 ` Segher Boessenkool 2021-06-03 9:18 ` Segher Boessenkool 2021-06-03 17:11 ` Jeff Law 2021-06-03 8:05 ` Richard Sandiford 1 sibling, 2 replies; 62+ messages in thread From: Segher Boessenkool @ 2021-06-03 8:00 UTC (permalink / raw) To: Kewen.Lin Cc: Richard Biener, Jeff Law, richard.sandiford, GCC Patches, Bill Schmidt, Jakub Jelinek Hi! On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote: > on 2021/6/3 上午7:52, Segher Boessenkool wrote: > >> - add a new "define_independent_insn_and_split" that has the > >> current semantics of define_insn_and_split. This should be > >> mechanical. > > > > I'd rather not have that -- we can just write separate define_insn and > > define_split in that case. > > Not sure if someone would argue that he/she would like to go with one shared > pattern as before, to avoid any possible differences between two seperated > patterns and have good maintainability (like only editing on place) and > slightly better efficiency. You only would do this if you have a different insn condition and split condition, which is a very important thing to know, it doesn't hurt to draw attention to it. The efficiency is exactly the same btw, define_insn_and_split is just syntactic sugar. The whole point of requiring the split condition to start with && is so it will become harder to mess things up (it will make the gen* code a tiny little bit simpler as well). And there is no transition period or anything like that needed either. Just the bunch that will break will need fixing. So let's find out how many of those there are :-) > > How many such cases *are* there? There are no users exposed to this, > > and when the split condition is required to start with "&&" (instead of > > getting that implied) it is not a silent change ever, either. > > If I read the proposal right, the explicit "&&" is only required when going > to find all potential problematic places for final implied "&&" change. > But one explicit "&&" does offer good readability. My proposal is to always require && (or maybe identical insn and split conditions should be allowed as well, if people still use that -- that is how we wrote "&& 1" before that existed). Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 8:00 ` Segher Boessenkool @ 2021-06-03 9:18 ` Segher Boessenkool 2021-06-04 2:57 ` Kewen.Lin 2021-06-03 17:11 ` Jeff Law 1 sibling, 1 reply; 62+ messages in thread From: Segher Boessenkool @ 2021-06-03 9:18 UTC (permalink / raw) To: Kewen.Lin; +Cc: Jakub Jelinek, richard.sandiford, Bill Schmidt, GCC Patches On Thu, Jun 03, 2021 at 03:00:44AM -0500, Segher Boessenkool wrote: > On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote: > The whole point of requiring the split condition to start with && is so > it will become harder to mess things up (it will make the gen* code a > tiny little bit simpler as well). And there is no transition period or > anything like that needed either. Just the bunch that will break will > need fixing. So let's find out how many of those there are :-) > > > > How many such cases *are* there? There are no users exposed to this, > > > and when the split condition is required to start with "&&" (instead of > > > getting that implied) it is not a silent change ever, either. > > > > If I read the proposal right, the explicit "&&" is only required when going > > to find all potential problematic places for final implied "&&" change. > > But one explicit "&&" does offer good readability. > > My proposal is to always require && (or maybe identical insn and split > conditions should be allowed as well, if people still use that -- that > is how we wrote "&& 1" before that existed). I prototyped this. There are very many errors. Iterators often modify the insn condition (for one iteration of it), but that does not work if the split condition does not start with "&&"! See attached prototype. Segher = = = diff --git a/gcc/gensupport.c b/gcc/gensupport.c index 2cb760ffb90f..05d46fd3775c 100644 --- a/gcc/gensupport.c +++ b/gcc/gensupport.c @@ -590,7 +590,6 @@ process_rtx (rtx desc, file_location loc) case DEFINE_INSN_AND_SPLIT: case DEFINE_INSN_AND_REWRITE: { - const char *split_cond; rtx split; rtvec attr; int i; @@ -611,15 +610,20 @@ process_rtx (rtx desc, file_location loc) /* If the split condition starts with "&&", append it to the insn condition to create the new split condition. */ - split_cond = XSTR (desc, 4); - if (split_cond[0] == '&' && split_cond[1] == '&') + const char *insn_cond = XSTR (desc, 2); + const char *split_cond = XSTR (desc, 4); + if (!strncmp (split_cond, "&&", 2)) { rtx_reader_ptr->copy_md_ptr_loc (split_cond + 2, split_cond); - split_cond = rtx_reader_ptr->join_c_conditions (XSTR (desc, 2), + split_cond = rtx_reader_ptr->join_c_conditions (insn_cond, split_cond + 2); + } else if (insn_cond[0]) { + if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) + error_at (loc, "the rewrite condition must start with `&&'"); + else + error_at (loc, "the split condition must start with `&&' [%s]", insn_cond); } - else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) - error_at (loc, "the rewrite condition must start with `&&'"); + XSTR (split, 1) = split_cond; if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 9:18 ` Segher Boessenkool @ 2021-06-04 2:57 ` Kewen.Lin 2021-06-07 7:12 ` Richard Biener 2021-06-07 23:50 ` Segher Boessenkool 0 siblings, 2 replies; 62+ messages in thread From: Kewen.Lin @ 2021-06-04 2:57 UTC (permalink / raw) To: Segher Boessenkool Cc: Jakub Jelinek, richard.sandiford, Bill Schmidt, GCC Patches [-- Attachment #1: Type: text/plain, Size: 3906 bytes --] Hi Segher, on 2021/6/3 下午5:18, Segher Boessenkool wrote: > On Thu, Jun 03, 2021 at 03:00:44AM -0500, Segher Boessenkool wrote: >> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote: >> The whole point of requiring the split condition to start with && is so >> it will become harder to mess things up (it will make the gen* code a >> tiny little bit simpler as well). And there is no transition period or >> anything like that needed either. Just the bunch that will break will >> need fixing. So let's find out how many of those there are :-) >> To find out those need fixing seems to be the critical part. It's not hard to add one explicit "&&" to those that don't have it now, but even with further bootstrapped and regression tested I'm still not confident the adjustments are safe enough, since the testing coverage could be limited. It may need more efforts to revisit, or/and test with more coverages, and port maintainers' reviews. In order to find one example which needs more fixing, for rs6000/i386/ aarch64, I fixed all define_insn_and_splits whose insn cond isn't empty (from gensupport's view since the iterator can add more on) while split cond don't start with "&&" , also skipped those whose insn conds are the same as their split conds. Unfortunately (or fortunately :-\) all were bootstrapped and regress-tested. The related diffs are attached, which is based on r12-0. >>>> How many such cases *are* there? There are no users exposed to this, >>>> and when the split condition is required to start with "&&" (instead of >>>> getting that implied) it is not a silent change ever, either. >>> >>> If I read the proposal right, the explicit "&&" is only required when going >>> to find all potential problematic places for final implied "&&" change. >>> But one explicit "&&" does offer good readability. >> >> My proposal is to always require && (or maybe identical insn and split >> conditions should be allowed as well, if people still use that -- that >> is how we wrote "&& 1" before that existed). > > I prototyped this. There are very many errors. Iterators often modify > the insn condition (for one iteration of it), but that does not work if > the split condition does not start with "&&"! > > See attached prototype. > > Thanks for the prototype! BR, Kewen > Segher > > = = = > > diff --git a/gcc/gensupport.c b/gcc/gensupport.c > index 2cb760ffb90f..05d46fd3775c 100644 > --- a/gcc/gensupport.c > +++ b/gcc/gensupport.c > @@ -590,7 +590,6 @@ process_rtx (rtx desc, file_location loc) > case DEFINE_INSN_AND_SPLIT: > case DEFINE_INSN_AND_REWRITE: > { > - const char *split_cond; > rtx split; > rtvec attr; > int i; > @@ -611,15 +610,20 @@ process_rtx (rtx desc, file_location loc) > > /* If the split condition starts with "&&", append it to the > insn condition to create the new split condition. */ > - split_cond = XSTR (desc, 4); > - if (split_cond[0] == '&' && split_cond[1] == '&') > + const char *insn_cond = XSTR (desc, 2); > + const char *split_cond = XSTR (desc, 4); > + if (!strncmp (split_cond, "&&", 2)) > { > rtx_reader_ptr->copy_md_ptr_loc (split_cond + 2, split_cond); > - split_cond = rtx_reader_ptr->join_c_conditions (XSTR (desc, 2), > + split_cond = rtx_reader_ptr->join_c_conditions (insn_cond, > split_cond + 2); > + } else if (insn_cond[0]) { > + if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > + error_at (loc, "the rewrite condition must start with `&&'"); > + else > + error_at (loc, "the split condition must start with `&&' [%s]", insn_cond); > } > - else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > - error_at (loc, "the rewrite condition must start with `&&'"); > + > XSTR (split, 1) = split_cond; > if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); > [-- Attachment #2: aarch64.diff --] [-- Type: text/plain, Size: 1063 bytes --] diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index abfd845..86869d9 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -1283,7 +1283,7 @@ (define_insn_and_split "*movsi_aarch64" fmov\\t%w0, %s1 fmov\\t%s0, %s1 * return aarch64_output_scalar_simd_mov_immediate (operands[1], SImode);" - "CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), SImode) + "&& CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), SImode) && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))" [(const_int 0)] "{ @@ -1319,7 +1319,7 @@ (define_insn_and_split "*movdi_aarch64" fmov\\t%x0, %d1 fmov\\t%d0, %d1 * return aarch64_output_scalar_simd_mov_immediate (operands[1], DImode);" - "(CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), DImode)) + "&& (CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), DImode)) && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))" [(const_int 0)] "{ [-- Attachment #3: i386.diff --] [-- Type: text/plain, Size: 11811 bytes --] diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index 9ff35d9..8d72f34 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -5249,7 +5249,7 @@ (define_insn_and_split "*add<dwi>3_doubleword" (clobber (reg:CC FLAGS_REG))] "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (reg:CCC FLAGS_REG) (compare:CCC (plus:DWIH (match_dup 1) (match_dup 2)) @@ -6050,7 +6050,7 @@ (define_insn_and_split "*addv<dwi>4_doubleword" (plus:<DWI> (match_dup 1) (match_dup 2)))] "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (reg:CCC FLAGS_REG) (compare:CCC (plus:DWIH (match_dup 1) (match_dup 2)) @@ -6097,7 +6097,7 @@ (define_insn_and_split "*addv<dwi>4_doubleword_1" && CONST_SCALAR_INT_P (operands[2]) && rtx_equal_p (operands[2], operands[3])" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (reg:CCC FLAGS_REG) (compare:CCC (plus:DWIH (match_dup 1) (match_dup 2)) @@ -6391,7 +6391,7 @@ (define_insn_and_split "*sub<dwi>3_doubleword" (clobber (reg:CC FLAGS_REG))] "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (reg:CC FLAGS_REG) (compare:CC (match_dup 1) (match_dup 2))) (set (match_dup 0) @@ -6559,7 +6559,7 @@ (define_insn_and_split "*subv<dwi>4_doubleword" (minus:<DWI> (match_dup 1) (match_dup 2)))] "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (reg:CC FLAGS_REG) (compare:CC (match_dup 1) (match_dup 2))) (set (match_dup 0) @@ -6604,7 +6604,7 @@ (define_insn_and_split "*subv<dwi>4_doubleword_1" && CONST_SCALAR_INT_P (operands[2]) && rtx_equal_p (operands[2], operands[3])" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (reg:CC FLAGS_REG) (compare:CC (match_dup 1) (match_dup 2))) (set (match_dup 0) @@ -7204,7 +7204,7 @@ (define_insn_and_split "*add<dwi>3_doubleword_cc_overflow_1" (plus:<DWI> (match_dup 1) (match_dup 2)))] "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (reg:CCC FLAGS_REG) (compare:CCC (plus:DWIH (match_dup 1) (match_dup 2)) @@ -8161,7 +8161,7 @@ (define_insn_and_split "divmod<mode>4_1" (clobber (reg:CC FLAGS_REG))] "" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (match_dup 1) (ashiftrt:SWI48 (match_dup 4) (match_dup 5))) (clobber (reg:CC FLAGS_REG))]) @@ -8196,7 +8196,7 @@ (define_insn_and_split "udivmod<mode>4_1" (clobber (reg:CC FLAGS_REG))] "" "#" - "reload_completed" + "&& reload_completed" [(set (match_dup 1) (const_int 0)) (parallel [(set (match_dup 0) (udiv:SWI48 (match_dup 2) (match_dup 3))) @@ -8336,7 +8336,7 @@ (define_insn_and_split "*divmod<mode>4" (clobber (reg:CC FLAGS_REG))] "" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (match_dup 1) (ashiftrt:SWIM248 (match_dup 4) (match_dup 5))) (clobber (reg:CC FLAGS_REG))]) @@ -8371,7 +8371,7 @@ (define_insn_and_split "*udivmod<mode>4" (clobber (reg:CC FLAGS_REG))] "" "#" - "reload_completed" + "&& reload_completed" [(set (match_dup 1) (const_int 0)) (parallel [(set (match_dup 0) (udiv:SWIM248 (match_dup 2) (match_dup 3))) @@ -10069,7 +10069,7 @@ (define_insn_and_split "*neg<dwi>2_doubleword" (clobber (reg:CC FLAGS_REG))] "ix86_unary_operator_ok (NEG, <DWI>mode, operands)" "#" - "reload_completed" + "&& reload_completed" [(parallel [(set (reg:CCC FLAGS_REG) (ne:CCC (match_dup 1) (const_int 0))) @@ -11545,7 +11545,7 @@ (define_insn_and_split "*<insn><mode>3_doubleword" (clobber (reg:CC FLAGS_REG))] "" "#" - "epilogue_completed" + "&& epilogue_completed" [(const_int 0)] "ix86_split_<insn> (operands, NULL_RTX, <MODE>mode); DONE;" [(set_attr "type" "multi")]) @@ -12045,7 +12045,7 @@ (define_insn_and_split "ix86_rotl<dwi>3_doubleword" (clobber (match_scratch:DWIH 3 "=&r"))] "" "#" - "reload_completed" + "&& reload_completed" [(set (match_dup 3) (match_dup 4)) (parallel [(set (match_dup 4) @@ -12073,7 +12073,7 @@ (define_insn_and_split "ix86_rotr<dwi>3_doubleword" (clobber (match_scratch:DWIH 3 "=&r"))] "" "#" - "reload_completed" + "&& reload_completed" [(set (match_dup 3) (match_dup 4)) (parallel [(set (match_dup 4) @@ -14308,7 +14308,7 @@ (define_insn_and_split "ctz<mode>2" return "bsf{<imodesuffix>}\t{%1, %0|%0, %1}"; } - "(TARGET_BMI || TARGET_GENERIC) + "&& (TARGET_BMI || TARGET_GENERIC) && TARGET_AVOID_FALSE_DEP_FOR_BMI && epilogue_completed && optimize_function_for_speed_p (cfun) && !reg_mentioned_p (operands[0], operands[1])" @@ -15712,7 +15712,7 @@ (define_insn_and_split "*load_tp_x32_zext" (unspec:SI [(const_int 0)] UNSPEC_TP)))] "TARGET_X32" "#" - "" + "&& 1" [(set (match_dup 0) (zero_extend:DI (match_dup 1)))] { @@ -15750,7 +15750,7 @@ (define_insn_and_split "*add_tp_x32_zext" (clobber (reg:CC FLAGS_REG))] "TARGET_X32" "#" - "" + "&& 1" [(parallel [(set (match_dup 0) (zero_extend:DI @@ -15841,7 +15841,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_32" (clobber (reg:CC FLAGS_REG))] "!TARGET_64BIT && TARGET_GNU2_TLS" "#" - "" + "&& 1" [(set (match_dup 0) (match_dup 5))] { operands[5] = can_create_pseudo_p () ? gen_reg_rtx (Pmode) : operands[0]; @@ -15900,7 +15900,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_64_<mode>" (clobber (reg:CC FLAGS_REG))] "TARGET_64BIT && TARGET_GNU2_TLS" "#" - "" + "&& 1" [(set (match_dup 0) (match_dup 4))] { operands[4] = can_create_pseudo_p () ? gen_reg_rtx (ptr_mode) : operands[0]; diff --git a/gcc/config/i386/mmx.md b/gcc/config/i386/mmx.md index 4c2b724..e6737b1 100644 --- a/gcc/config/i386/mmx.md +++ b/gcc/config/i386/mmx.md @@ -1668,7 +1668,7 @@ (define_insn_and_split "mmx_pack<s_trunsuffix>swb" pack<s_trunsuffix>swb\t{%2, %0|%0, %2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] "ix86_split_mmx_pack (operands, <any_s_truncate:CODE>); DONE;" @@ -1688,7 +1688,7 @@ (define_insn_and_split "mmx_packssdw" packssdw\t{%2, %0|%0, %2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] "ix86_split_mmx_pack (operands, SS_TRUNCATE); DONE;" @@ -1711,7 +1711,7 @@ (define_insn_and_split "mmx_punpckhbw" punpckhbw\t{%2, %0|%0, %2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] "ix86_split_mmx_punpck (operands, true); DONE;" @@ -1734,7 +1734,7 @@ (define_insn_and_split "mmx_punpcklbw" punpcklbw\t{%2, %0|%0, %k2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] "ix86_split_mmx_punpck (operands, false); DONE;" @@ -1755,7 +1755,7 @@ (define_insn_and_split "mmx_punpckhwd" punpckhwd\t{%2, %0|%0, %2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] "ix86_split_mmx_punpck (operands, true); DONE;" @@ -1776,7 +1776,7 @@ (define_insn_and_split "mmx_punpcklwd" punpcklwd\t{%2, %0|%0, %k2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] "ix86_split_mmx_punpck (operands, false); DONE;" @@ -1797,7 +1797,7 @@ (define_insn_and_split "mmx_punpckhdq" punpckhdq\t{%2, %0|%0, %2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] "ix86_split_mmx_punpck (operands, true); DONE;" @@ -1818,7 +1818,7 @@ (define_insn_and_split "mmx_punpckldq" punpckldq\t{%2, %0|%0, %k2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] "ix86_split_mmx_punpck (operands, false); DONE;" @@ -2542,7 +2542,7 @@ (define_insn_and_split "mmx_pmovmskb" "@ pmovmskb\t{%1, %0|%0, %1} #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REGNO_P (REGNO (operands[1]))" [(set (match_dup 0) (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)) diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md index 9d3728d..9919cc0 100644 --- a/gcc/config/i386/sse.md +++ b/gcc/config/i386/sse.md @@ -5223,7 +5223,7 @@ (define_insn_and_split "sse_cvtpi2ps" cvtpi2ps\t{%2, %0|%0, %2} # #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REG_P (operands[2])" [(const_int 0)] { @@ -5278,7 +5278,7 @@ (define_insn_and_split "sse_cvtps2pi" "@ cvtps2pi\t{%1, %0|%0, %q1} #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REG_P (operands[0])" [(const_int 0)] { @@ -5310,7 +5310,7 @@ (define_insn_and_split "sse_cvttps2pi" "@ cvttps2pi\t{%1, %0|%0, %q1} #" - "TARGET_SSE2 && reload_completed + "&& TARGET_SSE2 && reload_completed && SSE_REG_P (operands[0])" [(const_int 0)] { @@ -16467,7 +16467,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_lt" UNSPEC_MOVMSK))] "TARGET_SSE2" "#" - "" + "&& 1" [(set (match_dup 0) (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))] "" @@ -16489,7 +16489,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_zext_lt" UNSPEC_MOVMSK)))] "TARGET_64BIT && TARGET_SSE2" "#" - "" + "&& 1" [(set (match_dup 0) (zero_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))] "" @@ -16511,7 +16511,7 @@ (define_insn_and_split "*sse2_pmovmskb_ext_lt" UNSPEC_MOVMSK)))] "TARGET_64BIT && TARGET_SSE2" "#" - "" + "&& 1" [(set (match_dup 0) (sign_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))] "" @@ -16692,7 +16692,7 @@ (define_insn_and_split "ssse3_ph<plusminus_mnemonic>wv4hi3" ph<plusminus_mnemonic>w\t{%2, %0|%0, %2} # #" - "TARGET_SSSE3 && reload_completed + "&& TARGET_SSSE3 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] { @@ -16777,7 +16777,7 @@ (define_insn_and_split "ssse3_ph<plusminus_mnemonic>dv2si3" ph<plusminus_mnemonic>d\t{%2, %0|%0, %2} # #" - "TARGET_SSSE3 && reload_completed + "&& TARGET_SSSE3 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(const_int 0)] { @@ -17185,7 +17185,7 @@ (define_insn_and_split "*ssse3_pshufbv8qi3" pshufb\t{%2, %0|%0, %2} # #" - "TARGET_SSSE3 && reload_completed + "&& TARGET_SSSE3 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(set (match_dup 3) (and:V4SI (match_dup 3) (match_dup 2))) @@ -17315,7 +17315,7 @@ (define_insn_and_split "ssse3_palignrdi" gcc_unreachable (); } } - "TARGET_SSSE3 && reload_completed + "&& TARGET_SSSE3 && reload_completed && SSE_REGNO_P (REGNO (operands[0]))" [(set (match_dup 0) (lshiftrt:V1TI (match_dup 0) (match_dup 3)))] @@ -17769,7 +17769,7 @@ (define_insn_and_split "*<sse4_1_avx2>_pblendvb_lt" UNSPEC_BLENDV))] "TARGET_SSE4_1" "#" - "" + "&& 1" [(set (match_dup 0) (unspec:VI1_AVX2 [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPEC_BLENDV))] [-- Attachment #4: rs6000.diff --] [-- Type: text/plain, Size: 4522 bytes --] diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index 3f59b544f6a..feeeaffcc35 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -6752,7 +6752,7 @@ (define_insn_and_split "*and<mode>3_internal" return "#"; } - "reload_completed && int_reg_operand (operands[0], <MODE>mode)" + "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)" [(const_int 0)] { rs6000_split_logical (operands, AND, false, false, false); @@ -6788,7 +6788,7 @@ (define_insn_and_split "*bool<mode>3_internal" return "#"; } - "reload_completed && int_reg_operand (operands[0], <MODE>mode)" + "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)" [(const_int 0)] { rs6000_split_logical (operands, GET_CODE (operands[3]), false, false, false); @@ -6825,7 +6825,7 @@ (define_insn_and_split "*boolc<mode>3_internal1" return "#"; } - "(TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND)) + "&& (TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND)) && reload_completed && int_reg_operand (operands[0], <MODE>mode)" [(const_int 0)] { @@ -6854,7 +6854,7 @@ (define_insn_and_split "*boolc<mode>3_internal2" (match_operand:TI2 1 "int_reg_operand" "r,r,0")]))] "!TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)" "#" - "reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)" + "&& reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)" [(const_int 0)] { rs6000_split_logical (operands, GET_CODE (operands[3]), false, false, true); @@ -6885,7 +6885,7 @@ (define_insn_and_split "*boolcc<mode>3_internal1" return "#"; } - "(TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND)) + "&& (TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND)) && reload_completed && int_reg_operand (operands[0], <MODE>mode)" [(const_int 0)] { @@ -6915,7 +6915,7 @@ (define_insn_and_split "*boolcc<mode>3_internal2" (match_operand:TI2 2 "int_reg_operand" "r,r,0"))]))] "!TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)" "#" - "reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)" + "&& reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)" [(const_int 0)] { rs6000_split_logical (operands, GET_CODE (operands[3]), false, true, true); @@ -6943,7 +6943,7 @@ (define_insn_and_split "*eqv<mode>3_internal1" return "#"; } - "TARGET_P8_VECTOR && reload_completed + "&& TARGET_P8_VECTOR && reload_completed && int_reg_operand (operands[0], <MODE>mode)" [(const_int 0)] { @@ -6972,7 +6972,7 @@ (define_insn_and_split "*eqv<mode>3_internal2" (match_operand:TI2 2 "int_reg_operand" "r,r,0"))))] "!TARGET_P8_VECTOR" "#" - "reload_completed && !TARGET_P8_VECTOR" + "&& reload_completed && !TARGET_P8_VECTOR" [(const_int 0)] { rs6000_split_logical (operands, XOR, true, false, false); @@ -7000,7 +7000,7 @@ (define_insn_and_split "one_cmpl<mode>2" return "#"; } - "reload_completed && int_reg_operand (operands[0], <MODE>mode)" + "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)" [(const_int 0)] { rs6000_split_logical (operands, NOT, false, false, false); @@ -13532,7 +13532,7 @@ (define_insn_and_split "@eh_set_lr_<mode>" (clobber (match_scratch:P 1 "=&b"))] "" "#" - "reload_completed" + "&& reload_completed" [(const_int 0)] { rs6000_emit_eh_reg_restore (operands[0], operands[1]); diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md index bcb92be2f5c..2e0abb27354 100644 --- a/gcc/config/rs6000/vsx.md +++ b/gcc/config/rs6000/vsx.md @@ -1630,7 +1630,7 @@ (define_insn_and_split "vsx_mul_v2di" UNSPEC_VSX_MULSD))] "VECTOR_MEM_VSX_P (V2DImode)" "#" - "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed" + "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed" [(const_int 0)] { rtx op0 = operands[0]; @@ -1685,7 +1685,7 @@ (define_insn_and_split "vsx_div_v2di" UNSPEC_VSX_DIVSD))] "VECTOR_MEM_VSX_P (V2DImode)" "#" - "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed" + "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed" [(const_int 0)] { rtx op0 = operands[0]; @@ -1732,7 +1732,7 @@ (define_insn_and_split "vsx_udiv_v2di" UNSPEC_VSX_DIVUD))] "VECTOR_MEM_VSX_P (V2DImode)" "#" - "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed" + "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed" [(const_int 0)] { rtx op0 = operands[0]; ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-04 2:57 ` Kewen.Lin @ 2021-06-07 7:12 ` Richard Biener 2021-06-08 1:45 ` Kewen.Lin 2021-06-07 23:50 ` Segher Boessenkool 1 sibling, 1 reply; 62+ messages in thread From: Richard Biener @ 2021-06-07 7:12 UTC (permalink / raw) To: Kewen.Lin Cc: Segher Boessenkool, Jakub Jelinek, Richard Sandiford, Bill Schmidt, GCC Patches On Fri, Jun 4, 2021 at 4:58 AM Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi Segher, > > on 2021/6/3 下午5:18, Segher Boessenkool wrote: > > On Thu, Jun 03, 2021 at 03:00:44AM -0500, Segher Boessenkool wrote: > >> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote: > >> The whole point of requiring the split condition to start with && is so > >> it will become harder to mess things up (it will make the gen* code a > >> tiny little bit simpler as well). And there is no transition period or > >> anything like that needed either. Just the bunch that will break will > >> need fixing. So let's find out how many of those there are :-) > >> > > To find out those need fixing seems to be the critical part. It's > not hard to add one explicit "&&" to those that don't have it now, but > even with further bootstrapped and regression tested I'm still not > confident the adjustments are safe enough, since the testing coverage > could be limited. It may need more efforts to revisit, or/and test > with more coverages, and port maintainers' reviews. > > In order to find one example which needs more fixing, for rs6000/i386/ > aarch64, I fixed all define_insn_and_splits whose insn cond isn't empty > (from gensupport's view since the iterator can add more on) while split > cond don't start with "&&" , also skipped those whose insn conds are > the same as their split conds. Unfortunately (or fortunately :-\) all > were bootstrapped and regress-tested. > > The related diffs are attached, which is based on r12-0. For preserving existing behavior with requiring "&& " we can (mechanically?) split define_insn_and_split into define_insn and define_split with the old condition, right? That is, all empty insn condition cases are of course obvious to fix. > >>>> How many such cases *are* there? There are no users exposed to this, > >>>> and when the split condition is required to start with "&&" (instead of > >>>> getting that implied) it is not a silent change ever, either. > >>> > >>> If I read the proposal right, the explicit "&&" is only required when going > >>> to find all potential problematic places for final implied "&&" change. > >>> But one explicit "&&" does offer good readability. > >> > >> My proposal is to always require && (or maybe identical insn and split > >> conditions should be allowed as well, if people still use that -- that > >> is how we wrote "&& 1" before that existed). > > > > I prototyped this. There are very many errors. Iterators often modify > > the insn condition (for one iteration of it), but that does not work if > > the split condition does not start with "&&"! > > > > See attached prototype. > > > > > > Thanks for the prototype! > > BR, > Kewen > > > Segher > > > > = = = > > > > diff --git a/gcc/gensupport.c b/gcc/gensupport.c > > index 2cb760ffb90f..05d46fd3775c 100644 > > --- a/gcc/gensupport.c > > +++ b/gcc/gensupport.c > > @@ -590,7 +590,6 @@ process_rtx (rtx desc, file_location loc) > > case DEFINE_INSN_AND_SPLIT: > > case DEFINE_INSN_AND_REWRITE: > > { > > - const char *split_cond; > > rtx split; > > rtvec attr; > > int i; > > @@ -611,15 +610,20 @@ process_rtx (rtx desc, file_location loc) > > > > /* If the split condition starts with "&&", append it to the > > insn condition to create the new split condition. */ > > - split_cond = XSTR (desc, 4); > > - if (split_cond[0] == '&' && split_cond[1] == '&') > > + const char *insn_cond = XSTR (desc, 2); > > + const char *split_cond = XSTR (desc, 4); > > + if (!strncmp (split_cond, "&&", 2)) > > { > > rtx_reader_ptr->copy_md_ptr_loc (split_cond + 2, split_cond); > > - split_cond = rtx_reader_ptr->join_c_conditions (XSTR (desc, 2), > > + split_cond = rtx_reader_ptr->join_c_conditions (insn_cond, > > split_cond + 2); > > + } else if (insn_cond[0]) { > > + if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > > + error_at (loc, "the rewrite condition must start with `&&'"); > > + else > > + error_at (loc, "the split condition must start with `&&' [%s]", insn_cond); > > } > > - else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > > - error_at (loc, "the rewrite condition must start with `&&'"); > > + > > XSTR (split, 1) = split_cond; > > if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > > XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); > > ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-07 7:12 ` Richard Biener @ 2021-06-08 1:45 ` Kewen.Lin 0 siblings, 0 replies; 62+ messages in thread From: Kewen.Lin @ 2021-06-08 1:45 UTC (permalink / raw) To: Richard Biener Cc: Segher Boessenkool, Jakub Jelinek, Richard Sandiford, Bill Schmidt, GCC Patches on 2021/6/7 下午3:12, Richard Biener wrote: > On Fri, Jun 4, 2021 at 4:58 AM Kewen.Lin via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> Hi Segher, >> >> on 2021/6/3 下午5:18, Segher Boessenkool wrote: >>> On Thu, Jun 03, 2021 at 03:00:44AM -0500, Segher Boessenkool wrote: >>>> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote: >>>> The whole point of requiring the split condition to start with && is so >>>> it will become harder to mess things up (it will make the gen* code a >>>> tiny little bit simpler as well). And there is no transition period or >>>> anything like that needed either. Just the bunch that will break will >>>> need fixing. So let's find out how many of those there are :-) >>>> >> >> To find out those need fixing seems to be the critical part. It's >> not hard to add one explicit "&&" to those that don't have it now, but >> even with further bootstrapped and regression tested I'm still not >> confident the adjustments are safe enough, since the testing coverage >> could be limited. It may need more efforts to revisit, or/and test >> with more coverages, and port maintainers' reviews. >> >> In order to find one example which needs more fixing, for rs6000/i386/ >> aarch64, I fixed all define_insn_and_splits whose insn cond isn't empty >> (from gensupport's view since the iterator can add more on) while split >> cond don't start with "&&" , also skipped those whose insn conds are >> the same as their split conds. Unfortunately (or fortunately :-\) all >> were bootstrapped and regress-tested. >> >> The related diffs are attached, which is based on r12-0. > > For preserving existing behavior with requiring "&& " we can (mechanically?) > split define_insn_and_split into define_insn and define_split with the old > condition, right? > Yeah, we can replace all possible "unsafe" ones with separated define_insn and define_split. > That is, all empty insn condition cases are of course obvious to fix. > Sorry for the confusion, those ones are false positive. Mode iterators can append more conditions onto empty insn condition to make it not empty, I noticed it happened for split condition at the same time. The previous checking is very rough and didn't care about them as they are not harmful for the testing. My latest local patch has excluded them by checking the beginning and the end, also for the variant with one more external "()". BR, Kewen >>>>>> How many such cases *are* there? There are no users exposed to this, >>>>>> and when the split condition is required to start with "&&" (instead of >>>>>> getting that implied) it is not a silent change ever, either. >>>>> >>>>> If I read the proposal right, the explicit "&&" is only required when going >>>>> to find all potential problematic places for final implied "&&" change. >>>>> But one explicit "&&" does offer good readability. >>>> >>>> My proposal is to always require && (or maybe identical insn and split >>>> conditions should be allowed as well, if people still use that -- that >>>> is how we wrote "&& 1" before that existed). >>> >>> I prototyped this. There are very many errors. Iterators often modify >>> the insn condition (for one iteration of it), but that does not work if >>> the split condition does not start with "&&"! >>> >>> See attached prototype. >>> >>> >> >> Thanks for the prototype! >> >> BR, >> Kewen >> >>> Segher >>> >>> = = = >>> >>> diff --git a/gcc/gensupport.c b/gcc/gensupport.c >>> index 2cb760ffb90f..05d46fd3775c 100644 >>> --- a/gcc/gensupport.c >>> +++ b/gcc/gensupport.c >>> @@ -590,7 +590,6 @@ process_rtx (rtx desc, file_location loc) >>> case DEFINE_INSN_AND_SPLIT: >>> case DEFINE_INSN_AND_REWRITE: >>> { >>> - const char *split_cond; >>> rtx split; >>> rtvec attr; >>> int i; >>> @@ -611,15 +610,20 @@ process_rtx (rtx desc, file_location loc) >>> >>> /* If the split condition starts with "&&", append it to the >>> insn condition to create the new split condition. */ >>> - split_cond = XSTR (desc, 4); >>> - if (split_cond[0] == '&' && split_cond[1] == '&') >>> + const char *insn_cond = XSTR (desc, 2); >>> + const char *split_cond = XSTR (desc, 4); >>> + if (!strncmp (split_cond, "&&", 2)) >>> { >>> rtx_reader_ptr->copy_md_ptr_loc (split_cond + 2, split_cond); >>> - split_cond = rtx_reader_ptr->join_c_conditions (XSTR (desc, 2), >>> + split_cond = rtx_reader_ptr->join_c_conditions (insn_cond, >>> split_cond + 2); >>> + } else if (insn_cond[0]) { >>> + if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) >>> + error_at (loc, "the rewrite condition must start with `&&'"); >>> + else >>> + error_at (loc, "the split condition must start with `&&' [%s]", insn_cond); >>> } >>> - else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) >>> - error_at (loc, "the rewrite condition must start with `&&'"); >>> + >>> XSTR (split, 1) = split_cond; >>> if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) >>> XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); >>> ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-04 2:57 ` Kewen.Lin 2021-06-07 7:12 ` Richard Biener @ 2021-06-07 23:50 ` Segher Boessenkool 2021-06-08 2:09 ` Kewen.Lin 1 sibling, 1 reply; 62+ messages in thread From: Segher Boessenkool @ 2021-06-07 23:50 UTC (permalink / raw) To: Kewen.Lin; +Cc: Jakub Jelinek, richard.sandiford, Bill Schmidt, GCC Patches Hi! On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote: > To find out those need fixing seems to be the critical part. It's > not hard to add one explicit "&&" to those that don't have it now, but > even with further bootstrapped and regression tested I'm still not > confident the adjustments are safe enough, since the testing coverage > could be limited. It may need more efforts to revisit, or/and test > with more coverages, and port maintainers' reviews. https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html This adds an "&&" everywhere (or in fact, it just skips any existing one, it just has the same effect of adding it everywhere). I tested it with building gcc and Linux for all supported targets (31 of them; I do some with multiple configs, mostly 32-bit and 64-bit). None had any difference before and after the change. So I am no longer worried that there will be any fallout from doing this. There are many things that *could* go wrong, but I don't think there will be enough at all to be an impediment to just throwing the switch. If we go this way no target will need any significant fixing, maybe none at all will be needed across all targets. And no changes will be needed anywhere immediately. We could make leading "&&" deprecated, and the same for split condition "1" (which was "&& 1"). This is easy to change automatically as well. Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-07 23:50 ` Segher Boessenkool @ 2021-06-08 2:09 ` Kewen.Lin 2021-06-08 7:08 ` Richard Biener 0 siblings, 1 reply; 62+ messages in thread From: Kewen.Lin @ 2021-06-08 2:09 UTC (permalink / raw) To: Segher Boessenkool Cc: Jakub Jelinek, richard.sandiford, Bill Schmidt, GCC Patches Hi Segher, on 2021/6/8 上午7:50, Segher Boessenkool wrote: > Hi! > > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote: >> To find out those need fixing seems to be the critical part. It's >> not hard to add one explicit "&&" to those that don't have it now, but >> even with further bootstrapped and regression tested I'm still not >> confident the adjustments are safe enough, since the testing coverage >> could be limited. It may need more efforts to revisit, or/and test >> with more coverages, and port maintainers' reviews. > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html > > This adds an "&&" everywhere (or in fact, it just skips any existing > one, it just has the same effect of adding it everywhere). I tested it > with building gcc and Linux for all supported targets (31 of them; I do > some with multiple configs, mostly 32-bit and 64-bit). None had any > difference before and after the change. > > So I am no longer worried that there will be any fallout from doing > this. There are many things that *could* go wrong, but I don't think > there will be enough at all to be an impediment to just throwing the > switch. > > If we go this way no target will need any significant fixing, maybe none > at all will be needed across all targets. And no changes will be needed > anywhere immediately. We could make leading "&&" deprecated, and the > same for split condition "1" (which was "&& 1"). This is easy to change > automatically as well. > Thanks very much for doing this! I guess we are not going to backport this? If we won't, it seems to need some way to ensure the implied "&&" will show up explicitly when backporting some define_insn_and_split. BR, Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-08 2:09 ` Kewen.Lin @ 2021-06-08 7:08 ` Richard Biener 2021-06-08 12:30 ` Segher Boessenkool 0 siblings, 1 reply; 62+ messages in thread From: Richard Biener @ 2021-06-08 7:08 UTC (permalink / raw) To: Kewen.Lin Cc: Segher Boessenkool, Jakub Jelinek, Richard Sandiford, Bill Schmidt, GCC Patches On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi Segher, > > on 2021/6/8 上午7:50, Segher Boessenkool wrote: > > Hi! > > > > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote: > >> To find out those need fixing seems to be the critical part. It's > >> not hard to add one explicit "&&" to those that don't have it now, but > >> even with further bootstrapped and regression tested I'm still not > >> confident the adjustments are safe enough, since the testing coverage > >> could be limited. It may need more efforts to revisit, or/and test > >> with more coverages, and port maintainers' reviews. > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html > > > > This adds an "&&" everywhere (or in fact, it just skips any existing > > one, it just has the same effect of adding it everywhere). I tested it > > with building gcc and Linux for all supported targets (31 of them; I do > > some with multiple configs, mostly 32-bit and 64-bit). None had any > > difference before and after the change. > > > > So I am no longer worried that there will be any fallout from doing > > this. There are many things that *could* go wrong, but I don't think > > there will be enough at all to be an impediment to just throwing the > > switch. > > > > If we go this way no target will need any significant fixing, maybe none > > at all will be needed across all targets. And no changes will be needed > > anywhere immediately. We could make leading "&&" deprecated, and the > > same for split condition "1" (which was "&& 1"). This is easy to change > > automatically as well. > > > Thanks very much for doing this! > > I guess we are not going to backport this? If we won't, it seems to need > some way to ensure the implied "&&" will show up explicitly when backporting > some define_insn_and_split. For this reason I'd prefer the explicit "&& ", Seghers testing means mass-changing all define_insn_and_split is reasonable. Richard. > BR, > Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-08 7:08 ` Richard Biener @ 2021-06-08 12:30 ` Segher Boessenkool 2021-06-08 12:50 ` Richard Biener 0 siblings, 1 reply; 62+ messages in thread From: Segher Boessenkool @ 2021-06-08 12:30 UTC (permalink / raw) To: Richard Biener Cc: Kewen.Lin, Jakub Jelinek, Richard Sandiford, Bill Schmidt, GCC Patches On Tue, Jun 08, 2021 at 09:08:56AM +0200, Richard Biener wrote: > On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > on 2021/6/8 上午7:50, Segher Boessenkool wrote: > > > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote: > > >> To find out those need fixing seems to be the critical part. It's > > >> not hard to add one explicit "&&" to those that don't have it now, but > > >> even with further bootstrapped and regression tested I'm still not > > >> confident the adjustments are safe enough, since the testing coverage > > >> could be limited. It may need more efforts to revisit, or/and test > > >> with more coverages, and port maintainers' reviews. > > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html > > > > > > This adds an "&&" everywhere (or in fact, it just skips any existing > > > one, it just has the same effect of adding it everywhere). I tested it > > > with building gcc and Linux for all supported targets (31 of them; I do > > > some with multiple configs, mostly 32-bit and 64-bit). None had any > > > difference before and after the change. > > > > > > So I am no longer worried that there will be any fallout from doing > > > this. There are many things that *could* go wrong, but I don't think > > > there will be enough at all to be an impediment to just throwing the > > > switch. > > > > > > If we go this way no target will need any significant fixing, maybe none > > > at all will be needed across all targets. And no changes will be needed > > > anywhere immediately. We could make leading "&&" deprecated, and the > > > same for split condition "1" (which was "&& 1"). This is easy to change > > > automatically as well. > > > > > Thanks very much for doing this! > > > > I guess we are not going to backport this? If we won't, it seems to need > > some way to ensure the implied "&&" will show up explicitly when backporting > > some define_insn_and_split. > > For this reason I'd prefer the explicit "&& ", Seghers testing means > mass-changing all define_insn_and_split is reasonable. So mass-change all define_insn_and_split where the split condition is not inclusive of the insn condition (as written, i.e. before the iterators have added stuff), to be separate define_insn and define_split patterns? And *then* add the &&? Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-08 12:30 ` Segher Boessenkool @ 2021-06-08 12:50 ` Richard Biener 0 siblings, 0 replies; 62+ messages in thread From: Richard Biener @ 2021-06-08 12:50 UTC (permalink / raw) To: Segher Boessenkool Cc: Kewen.Lin, Jakub Jelinek, Richard Sandiford, Bill Schmidt, GCC Patches On Tue, Jun 8, 2021 at 2:32 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Tue, Jun 08, 2021 at 09:08:56AM +0200, Richard Biener wrote: > > On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > > > on 2021/6/8 上午7:50, Segher Boessenkool wrote: > > > > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote: > > > >> To find out those need fixing seems to be the critical part. It's > > > >> not hard to add one explicit "&&" to those that don't have it now, but > > > >> even with further bootstrapped and regression tested I'm still not > > > >> confident the adjustments are safe enough, since the testing coverage > > > >> could be limited. It may need more efforts to revisit, or/and test > > > >> with more coverages, and port maintainers' reviews. > > > > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html > > > > > > > > This adds an "&&" everywhere (or in fact, it just skips any existing > > > > one, it just has the same effect of adding it everywhere). I tested it > > > > with building gcc and Linux for all supported targets (31 of them; I do > > > > some with multiple configs, mostly 32-bit and 64-bit). None had any > > > > difference before and after the change. > > > > > > > > So I am no longer worried that there will be any fallout from doing > > > > this. There are many things that *could* go wrong, but I don't think > > > > there will be enough at all to be an impediment to just throwing the > > > > switch. > > > > > > > > If we go this way no target will need any significant fixing, maybe none > > > > at all will be needed across all targets. And no changes will be needed > > > > anywhere immediately. We could make leading "&&" deprecated, and the > > > > same for split condition "1" (which was "&& 1"). This is easy to change > > > > automatically as well. > > > > > > > Thanks very much for doing this! > > > > > > I guess we are not going to backport this? If we won't, it seems to need > > > some way to ensure the implied "&&" will show up explicitly when backporting > > > some define_insn_and_split. > > > > For this reason I'd prefer the explicit "&& ", Seghers testing means > > mass-changing all define_insn_and_split is reasonable. > > So mass-change all define_insn_and_split where the split condition is > not inclusive of the insn condition (as written, i.e. before the > iterators have added stuff), to be separate define_insn and define_split > patterns? And *then* add the &&? Possible. Maybe first enable a warning for the case with not starting with "&& " to give maintainers a chance to fix them and only then do the mass change to separate define_insn and define_split to avoid churn in the .md files. Well maintained ports should be quick to fixup themselves. And some cases might even be obvious. Richard. > > Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 8:00 ` Segher Boessenkool 2021-06-03 9:18 ` Segher Boessenkool @ 2021-06-03 17:11 ` Jeff Law 2021-06-03 22:19 ` Segher Boessenkool 1 sibling, 1 reply; 62+ messages in thread From: Jeff Law @ 2021-06-03 17:11 UTC (permalink / raw) To: Segher Boessenkool, Kewen.Lin Cc: Richard Biener, richard.sandiford, GCC Patches, Bill Schmidt, Jakub Jelinek On 6/3/2021 2:00 AM, Segher Boessenkool wrote: > Hi! > > On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote: >> on 2021/6/3 上午7:52, Segher Boessenkool wrote: >>>> - add a new "define_independent_insn_and_split" that has the >>>> current semantics of define_insn_and_split. This should be >>>> mechanical. >>> I'd rather not have that -- we can just write separate define_insn and >>> define_split in that case. >> Not sure if someone would argue that he/she would like to go with one shared >> pattern as before, to avoid any possible differences between two seperated >> patterns and have good maintainability (like only editing on place) and >> slightly better efficiency. > You only would do this if you have a different insn condition and split > condition, which is a very important thing to know, it doesn't hurt to > draw attention to it. The efficiency is exactly the same btw, > define_insn_and_split is just syntactic sugar. > > The whole point of requiring the split condition to start with && is so > it will become harder to mess things up (it will make the gen* code a > tiny little bit simpler as well). And there is no transition period or > anything like that needed either. Just the bunch that will break will > need fixing. So let's find out how many of those there are :-) Exactly. While these empty conditions or those not starting with "&&" are technically valid, they're all suspicious from a port correctness standpoint, particularly if the main condition is non-empty. Having made that mistake when converting the H8 away from CC0, I can say fairly confidently that if we had this in place a year ago that those mistakes would likely have been avoided. Thankfully the H8 isn't a heavily used port and has limped along until I stumbled over the issue a week or so ago while polishing some improvements to the port. Jeff ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 17:11 ` Jeff Law @ 2021-06-03 22:19 ` Segher Boessenkool 0 siblings, 0 replies; 62+ messages in thread From: Segher Boessenkool @ 2021-06-03 22:19 UTC (permalink / raw) To: Jeff Law Cc: Kewen.Lin, Richard Biener, richard.sandiford, GCC Patches, Bill Schmidt, Jakub Jelinek On Thu, Jun 03, 2021 at 11:11:53AM -0600, Jeff Law wrote: > On 6/3/2021 2:00 AM, Segher Boessenkool wrote: > >The whole point of requiring the split condition to start with && is so > >it will become harder to mess things up (it will make the gen* code a > >tiny little bit simpler as well). And there is no transition period or > >anything like that needed either. Just the bunch that will break will > >need fixing. So let's find out how many of those there are :-) > Exactly. While these empty conditions or those not starting with "&&" > are technically valid, they're all suspicious from a port correctness > standpoint, particularly if the main condition is non-empty. And note that this is also the case if you wrote the insn condition as an empty string, but you used some iterator with a condition. I found many of these in rs6000. This will need to be fixed before we do anything else. > Having made that mistake when converting the H8 away from CC0, I can say > fairly confidently that if we had this in place a year ago that those > mistakes would likely have been avoided. Thankfully the H8 isn't a > heavily used port and has limped along until I stumbled over the issue a > week or so ago while polishing some improvements to the port. I've noticed unexpected splits in rs6000 only a few times over the years. That doesn't mean more didn't happen, they just didn't cause obvious enough problems :-) Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 5:22 ` Kewen.Lin 2021-06-03 8:00 ` Segher Boessenkool @ 2021-06-03 8:05 ` Richard Sandiford 2021-06-03 10:01 ` Segher Boessenkool 2021-06-04 3:33 ` Kewen.Lin 1 sibling, 2 replies; 62+ messages in thread From: Richard Sandiford @ 2021-06-03 8:05 UTC (permalink / raw) To: Kewen.Lin Cc: Segher Boessenkool, Richard Biener, Jeff Law, GCC Patches, Bill Schmidt, Jakub Jelinek "Kewen.Lin" <linkw@linux.ibm.com> writes: > Hi Richi/Richard/Jeff/Segher, > > Thanks for the comments! > > on 2021/6/3 锟斤拷锟斤拷7:52, Segher Boessenkool wrote: >> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote: >>> Richard Biener <richard.guenther@gmail.com> writes: >>>> So what Richard suggests would be to disallow split conditions >>>> that do not start with "&& ", it's probably easy to do that as well >>>> and look for build fails. That should catch all cases to look at. >>> >>> Yeah. As a strawman proposal, how about: >>> >>> - add a new "define_independent_insn_and_split" that has the >>> current semantics of define_insn_and_split. This should be >>> mechanical. >> >> I'd rather not have that -- we can just write separate define_insn and >> define_split in that case. >> > > Not sure if someone would argue that he/she would like to go with one shared > pattern as before, to avoid any possible differences between two seperated > patterns and have good maintainability (like only editing on place) and > slightly better efficiency. Right. Plus it creates less make-work. If we didn't have it, someone would need to split the define_insn_and_splits that don't currently use "&&", then later someone might decide that the missing "&&" was a mistake and need to put them together again (or just revert the patch and edit from there, I guess). Plus define_independent_insn_and_split would act as a flag for something that might be suspect. If we split them then the define_split condition will seem to have been chosen deliberately in isolation. >> How many such cases *are* there? There are no users exposed to this, >> and when the split condition is required to start with "&&" (instead of >> getting that implied) it is not a silent change ever, either. >> > > If I read the proposal right, the explicit "&&" is only required when going > to find all potential problematic places for final implied "&&" change. > But one explicit "&&" does offer good readability. I don't know. "&& 1" looks kind of weird to me. One thing I'd been wondering about a while ago was whether we should key the split part of define_insn_and_splits off the insn code, instead of repeating the pattern match and insn C condition. That would make the split apply only to the associated define_insns, whereas at the moment they also apply to any earlier (less general) define_insn that happens to match the pattern and the C conditions. It would also reduce the complexity of the autogenerated define_split logic. I don't know whether that's a good idea or not. But having an explicit "&&" implies that the generator shouldn't do that, and that it should retest the insn condition from scratch. >>> - find the define_insn_and_splits that are missing the "&&", and where >>> missing the "&&" might make a difference. Change them to >>> define_independent_insn_and_splits. >>> >>> Like Richard says, this can be done by temporarily disallowing >>> define_insn_and_splits that have no "&&". >> >> If we make that change permanently, that is all steps we ever need! >> > > So the question is that: whether we need to demand an explicit "&&". > Richard's proposal is for answer "no" which aligns with Richi's auto > filling advice before. I think it would result in fewer changes since > those places without explicit "&&" are mostly unintentional, all the jobs > are done by implied "&&". Its downside seems to be bad readability, new > readers may take it as two seperated conditions at first glance, but I > guess if we emphasize this change in the document it would be fine? > Or emitting one warning if missing an explicit "&&"? IMO the natural way to read it is that the split C condition gives the conditions under which the instruction should be split. I think that's why forgetting the "&&" is such a common mistake. (I know I've done it plenty of times.) IMO requiring the "&&" is baking in an alternative, less intuitive, interpretation. Thanks, Richard ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 8:05 ` Richard Sandiford @ 2021-06-03 10:01 ` Segher Boessenkool 2021-06-03 10:25 ` Richard Sandiford 2021-06-04 3:33 ` Kewen.Lin 1 sibling, 1 reply; 62+ messages in thread From: Segher Boessenkool @ 2021-06-03 10:01 UTC (permalink / raw) To: Kewen.Lin, Richard Biener, Jeff Law, GCC Patches, Bill Schmidt, Jakub Jelinek, richard.sandiford On Thu, Jun 03, 2021 at 09:05:02AM +0100, Richard Sandiford via Gcc-patches wrote: > Right. Plus it creates less make-work. If we didn't have it, someone > would need to split the define_insn_and_splits that don't currently > use "&&", then later someone might decide that the missing "&&" was a > mistake and need to put them together again (or just revert the patch > and edit from there, I guess). I would hope no maintainer would be foolish enough to flip-flop like that. > Plus define_independent_insn_and_split would act as a flag for something > that might be suspect. That always is a conceptual error, even. The name says so already: the insn and the split are very different things, with different conditions, they just happen to share a pattern. > If we split them then the define_split condition > will seem to have been chosen deliberately in isolation. And it will have beenm chosen deliberately! Why *else* write things like this? > > If I read the proposal right, the explicit "&&" is only required when going > > to find all potential problematic places for final implied "&&" change. > > But one explicit "&&" does offer good readability. > > I don't know. "&& 1" looks kind of weird to me. We have it in rs6000.md since 2004. sparc has had it since 2002. i386 has had it since 2001. arm still does not have it :-) > > So the question is that: whether we need to demand an explicit "&&". > > Richard's proposal is for answer "no" which aligns with Richi's auto > > filling advice before. I think it would result in fewer changes since > > those places without explicit "&&" are mostly unintentional, all the jobs > > are done by implied "&&". Its downside seems to be bad readability, new > > readers may take it as two seperated conditions at first glance, but I > > guess if we emphasize this change in the document it would be fine? > > Or emitting one warning if missing an explicit "&&"? > > IMO the natural way to read it is that the split C condition gives the > conditions under which the instruction should be split. I think that's > why forgetting the "&&" is such a common mistake. (I know I've done it > plenty of times.) You even do in this description! :-) You do not want the define_split in a define_insn_and_split to happen for insns that do not match the insn condition, that would not be intuitive at all. > IMO requiring the "&&" is baking in an alternative, less intuitive, > interpretation. But you want the exact same semantics, right? I do agree that the syntax without "&&" looks nicer. It has many practical problems though, so it is nice to aim for, but we cannot move there until we have all existing machine descriptions fixed up first. Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 10:01 ` Segher Boessenkool @ 2021-06-03 10:25 ` Richard Sandiford 2021-06-03 21:25 ` Segher Boessenkool 0 siblings, 1 reply; 62+ messages in thread From: Richard Sandiford @ 2021-06-03 10:25 UTC (permalink / raw) To: Segher Boessenkool Cc: Kewen.Lin, Richard Biener, Jeff Law, GCC Patches, Bill Schmidt, Jakub Jelinek Segher Boessenkool <segher@kernel.crashing.org> writes: > On Thu, Jun 03, 2021 at 09:05:02AM +0100, Richard Sandiford via Gcc-patches wrote: >> Right. Plus it creates less make-work. If we didn't have it, someone >> would need to split the define_insn_and_splits that don't currently >> use "&&", then later someone might decide that the missing "&&" was a >> mistake and need to put them together again (or just revert the patch >> and edit from there, I guess). > > I would hope no maintainer would be foolish enough to flip-flop like > that. But the point is that the maintainers wouldn't be flip-flopping. The move to define_independent_insn_and_split would be a mechnical change made by someone else. We shouldn't just add "&&" to all define_insn_and_splits that currently lack them. That might well break things. So to get the perfect result, someone has to look at each individual define_insn_and_split that currently lacks a "&&" to see what the effects of adding "&&" would be. IMO it's not reasonable to ask Kewen to do that for all ports. So the process I suggested was a way of mechanically changing existing ports in a way that would not require input from target maintainers, or extensive testing on affected targets. It would keep the behaviour of the port the same as it is now (whether that's good or bad). This part of the process could be scripted. >> > If I read the proposal right, the explicit "&&" is only required when going >> > to find all potential problematic places for final implied "&&" change. >> > But one explicit "&&" does offer good readability. >> >> I don't know. "&& 1" looks kind of weird to me. > > We have it in rs6000.md since 2004. sparc has had it since 2002. i386 > has had it since 2001. arm still does not have it :-) Sure, this syntax goes back 20 years. I don't think that answers the point though. The question was whether a split condition "&& 1" is more readable than a condition "", given that "" means "always" in other contexts. Given the choice, IMO "" is more readable and "&& 1" looks weird/inconsistent. Thanks, Richard ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 10:25 ` Richard Sandiford @ 2021-06-03 21:25 ` Segher Boessenkool 2021-06-03 21:34 ` Jakub Jelinek 0 siblings, 1 reply; 62+ messages in thread From: Segher Boessenkool @ 2021-06-03 21:25 UTC (permalink / raw) To: Kewen.Lin, Richard Biener, Jeff Law, GCC Patches, Bill Schmidt, Jakub Jelinek, richard.sandiford On Thu, Jun 03, 2021 at 11:25:49AM +0100, Richard Sandiford wrote: > We shouldn't just add "&&" to all define_insn_and_splits that currently > lack them. My previous post shows that this *already* is required. > IMO it's not reasonable to ask Kewen to do that for all ports. So the > process I suggested was a way of mechanically changing existing ports in > a way that would not require input from target maintainers, or extensive > testing on affected targets. I fear it will end up as Yet Another unfinished transition this way :-( > >> I don't know. "&& 1" looks kind of weird to me. > > > > We have it in rs6000.md since 2004. sparc has had it since 2002. i386 > > has had it since 2001. arm still does not have it :-) > > Sure, this syntax goes back 20 years. I don't think that answers the > point though. The question was whether a split condition "&& 1" is > more readable than a condition "", given that "" means "always" in other > contexts. Given the choice, IMO "" is more readable and "&& 1" looks > weird/inconsistent. In most ports "&& 1" is all over the place and well-known already. Sure we could change to "" meaning always, but that is not what it currently means! If we could just start all over we could do it perfectly (but see second-system syndrome, heh). But we cannot. IMO we should especially avoid everything that uses new semantics for old syntax. Segher ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 21:25 ` Segher Boessenkool @ 2021-06-03 21:34 ` Jakub Jelinek 0 siblings, 0 replies; 62+ messages in thread From: Jakub Jelinek @ 2021-06-03 21:34 UTC (permalink / raw) To: Segher Boessenkool Cc: Kewen.Lin, Richard Biener, Jeff Law, GCC Patches, Bill Schmidt, richard.sandiford On Thu, Jun 03, 2021 at 04:25:44PM -0500, Segher Boessenkool wrote: > If we could just start all over we could do it perfectly (but see > second-system syndrome, heh). But we cannot. IMO we should especially > avoid everything that uses new semantics for old syntax. Agreed, that would be a nightmare for backporting. Jakub ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions 2021-06-03 8:05 ` Richard Sandiford 2021-06-03 10:01 ` Segher Boessenkool @ 2021-06-04 3:33 ` Kewen.Lin 1 sibling, 0 replies; 62+ messages in thread From: Kewen.Lin @ 2021-06-04 3:33 UTC (permalink / raw) To: richard.sandiford Cc: Segher Boessenkool, Jeff Law, Richard Biener, Jakub Jelinek, Bill Schmidt, GCC Patches on 2021/6/3 下午4:05, Richard Sandiford wrote: > "Kewen.Lin" <linkw@linux.ibm.com> writes: >> Hi Richi/Richard/Jeff/Segher, >> >> Thanks for the comments! >> >> on 2021/6/3 锟斤拷锟斤拷7:52, Segher Boessenkool wrote: >>> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote: >>>> Richard Biener <richard.guenther@gmail.com> writes: >>>>> So what Richard suggests would be to disallow split conditions >>>>> that do not start with "&& ", it's probably easy to do that as well >>>>> and look for build fails. That should catch all cases to look at. >>>> >>>> Yeah. As a strawman proposal, how about: >>>> >>>> - add a new "define_independent_insn_and_split" that has the >>>> current semantics of define_insn_and_split. This should be >>>> mechanical. >>> >>> I'd rather not have that -- we can just write separate define_insn and >>> define_split in that case. >>> >> >> Not sure if someone would argue that he/she would like to go with one shared >> pattern as before, to avoid any possible differences between two seperated >> patterns and have good maintainability (like only editing on place) and >> slightly better efficiency. > > Right. Plus it creates less make-work. If we didn't have it, someone > would need to split the define_insn_and_splits that don't currently > use "&&", then later someone might decide that the missing "&&" was a > mistake and need to put them together again (or just revert the patch > and edit from there, I guess). > > Plus define_independent_insn_and_split would act as a flag for something > that might be suspect. If we split them then the define_split condition > will seem to have been chosen deliberately in isolation. > >>> How many such cases *are* there? There are no users exposed to this, >>> and when the split condition is required to start with "&&" (instead of >>> getting that implied) it is not a silent change ever, either. >>> >> >> If I read the proposal right, the explicit "&&" is only required when going >> to find all potential problematic places for final implied "&&" change. >> But one explicit "&&" does offer good readability. > > I don't know. "&& 1" looks kind of weird to me. > > One thing I'd been wondering about a while ago was whether we should key > the split part of define_insn_and_splits off the insn code, instead of > repeating the pattern match and insn C condition. That would make the > split apply only to the associated define_insns, whereas at the moment > they also apply to any earlier (less general) define_insn that happens > to match the pattern and the C conditions. It would also reduce the > complexity of the autogenerated define_split logic. > > I don't know whether that's a good idea or not. But having an explicit > "&&" implies that the generator shouldn't do that, and that it should > retest the insn condition from scratch. > >>>> - find the define_insn_and_splits that are missing the "&&", and where >>>> missing the "&&" might make a difference. Change them to >>>> define_independent_insn_and_splits. >>>> >>>> Like Richard says, this can be done by temporarily disallowing >>>> define_insn_and_splits that have no "&&". >>> >>> If we make that change permanently, that is all steps we ever need! >>> >> >> So the question is that: whether we need to demand an explicit "&&". >> Richard's proposal is for answer "no" which aligns with Richi's auto >> filling advice before. I think it would result in fewer changes since >> those places without explicit "&&" are mostly unintentional, all the jobs >> are done by implied "&&". Its downside seems to be bad readability, new >> readers may take it as two seperated conditions at first glance, but I >> guess if we emphasize this change in the document it would be fine? >> Or emitting one warning if missing an explicit "&&"? > > IMO the natural way to read it is that the split C condition gives the > conditions under which the instruction should be split. I think that's > why forgetting the "&&" is such a common mistake. (I know I've done it > plenty of times.) > > IMO requiring the "&&" is baking in an alternative, less intuitive, > interpretation. > Thanks for the explanation, I was thinking people may have got used to the starting "&&" in split condition, so it's easy for them to read. But I agree it's better not to have it in the natural way. BR, Kewen ^ permalink raw reply [flat|nested] 62+ messages in thread
end of thread, other threads:[~2021-06-08 12:50 UTC | newest] Thread overview: 62+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-02 5:04 [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Kewen Lin 2021-06-02 5:04 ` [PATCH 01/11] gen: Emit error msg for empty split condition Kewen Lin 2021-06-02 7:04 ` Richard Biener 2021-06-02 7:27 ` Kewen.Lin 2021-06-02 7:43 ` Richard Biener 2021-06-02 8:18 ` Kewen.Lin 2021-06-02 23:35 ` Segher Boessenkool 2021-06-04 19:03 ` Martin Sebor 2021-06-04 19:37 ` Segher Boessenkool 2021-06-02 5:04 ` [PATCH 02/11] arc: Update unexpected " Kewen Lin 2021-06-02 6:52 ` Claudiu Zissulescu 2021-06-02 7:05 ` Kewen.Lin 2021-06-02 7:12 ` Claudiu Zissulescu 2021-06-02 7:43 ` [PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di Kewen.Lin 2021-06-02 8:33 ` Claudiu Zissulescu 2021-06-02 5:04 ` [PATCH 03/11] arm: Update unexpected empty split condition Kewen Lin 2021-06-02 9:02 ` Kyrylo Tkachov 2021-06-02 5:04 ` [PATCH 04/11] cris: " Kewen Lin 2021-06-02 12:45 ` Hans-Peter Nilsson 2021-06-03 5:45 ` Kewen.Lin 2021-06-03 16:12 ` Hans-Peter Nilsson 2021-06-03 22:33 ` Hans-Peter Nilsson 2021-06-04 3:25 ` Kewen.Lin 2021-06-02 5:04 ` [PATCH 05/11] h8300: " Kewen Lin 2021-06-02 17:10 ` Jeff Law 2021-06-02 5:04 ` [PATCH 06/11] i386: " Kewen Lin 2021-06-02 6:28 ` Uros Bizjak 2021-06-02 5:04 ` [PATCH 07/11] m68k: " Kewen Lin 2021-06-02 17:08 ` Jeff Law 2021-06-02 5:04 ` [PATCH 08/11] mips: " Kewen Lin 2021-06-02 17:11 ` Jeff Law 2021-06-02 5:04 ` [PATCH 09/11] or1k: " Kewen Lin 2021-06-02 5:05 ` [PATCH 10/11] sh: " Kewen Lin 2021-06-02 5:17 ` Oleg Endo 2021-06-02 5:05 ` [PATCH 11/11] sparc: " Kewen Lin 2021-06-02 8:11 ` [RFC/PATCH 00/11] Fix up some unexpected empty split conditions Richard Sandiford 2021-06-02 8:37 ` Kewen.Lin 2021-06-02 9:13 ` Richard Sandiford 2021-06-02 10:01 ` Kewen.Lin 2021-06-02 10:12 ` Richard Biener 2021-06-02 17:32 ` Richard Sandiford 2021-06-02 18:25 ` Jeff Law 2021-06-02 23:52 ` Segher Boessenkool 2021-06-03 5:22 ` Kewen.Lin 2021-06-03 8:00 ` Segher Boessenkool 2021-06-03 9:18 ` Segher Boessenkool 2021-06-04 2:57 ` Kewen.Lin 2021-06-07 7:12 ` Richard Biener 2021-06-08 1:45 ` Kewen.Lin 2021-06-07 23:50 ` Segher Boessenkool 2021-06-08 2:09 ` Kewen.Lin 2021-06-08 7:08 ` Richard Biener 2021-06-08 12:30 ` Segher Boessenkool 2021-06-08 12:50 ` Richard Biener 2021-06-03 17:11 ` Jeff Law 2021-06-03 22:19 ` Segher Boessenkool 2021-06-03 8:05 ` Richard Sandiford 2021-06-03 10:01 ` Segher Boessenkool 2021-06-03 10:25 ` Richard Sandiford 2021-06-03 21:25 ` Segher Boessenkool 2021-06-03 21:34 ` Jakub Jelinek 2021-06-04 3:33 ` Kewen.Lin
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).