| Message ID | c792c9b32d65d75dabede69732cbc74f138da6b3.1622179420.git.linkw@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix up some unexpected empty split conditions | expand |
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 >
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 >>
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 > >> >
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
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
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)); >
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
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));