[01/11] gen: Emit error msg for empty split condition

Message ID c792c9b32d65d75dabede69732cbc74f138da6b3.1622179420.git.linkw@linux.ibm.com
State New
Headers show
Series
  • Fix up some unexpected empty split conditions
Related show

Commit Message

liuhongt via Gcc-patches June 2, 2021, 5:04 a.m.
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(+)

-- 
2.17.1

Comments

liuhongt via Gcc-patches June 2, 2021, 7:04 a.m. | #1
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

>
liuhongt via Gcc-patches June 2, 2021, 7:27 a.m. | #2
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

>>
liuhongt via Gcc-patches June 2, 2021, 7:43 a.m. | #3
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

> >>

>
liuhongt via Gcc-patches June 2, 2021, 8:18 a.m. | #4
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
Segher Boessenkool June 2, 2021, 11:35 p.m. | #5
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
liuhongt via Gcc-patches June 4, 2021, 7:03 p.m. | #6
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));

>
Segher Boessenkool June 4, 2021, 7:37 p.m. | #7
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

Patch

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));