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