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

["Kewen.Lin" <linkw@linux.ibm.com>]

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