rs6000: Fix some unexpected empty split conditions

Hi,

As Segher and Mike pointed out, the define_insn_and_split should avoid
to use empty split condition if the condition for define_insn isn't empty,
otherwise it can sometimes leads to unexpected consequence.

This patch is to fix some places like this.

Bootstrapped/regtested on powerpc64le-linux-gnu P9 and
powerpc64-linux-gnu P8.

Since it's in very low risk and can avoid some unexpected errors,
is it ok for trunk? or has to be for GCC12?

BR,
Kewen
------
gcc/ChangeLog:

	* config/rs6000/rs6000.md (*rotldi3_insert_sf,
	*mov<SFDF:mode><SFDF2:mode>cc_p9, floatsi<mode>2_lfiwax,
	floatsi<mode>2_lfiwax_mem, floatunssi<mode>2_lfiwzx,
	floatunssi<mode>2_lfiwzx_mem, *floatsidf2_internal,
	*floatunssidf2_internal, fix_trunc<mode>si2_stfiwx,
	fix_trunc<mode>si2_internal, fixuns_trunc<mode>si2_stfiwx,
	*round32<mode>2_fprs, *roundu32<mode>2_fprs,
	*fix_trunc<mode>si2_internal): Fix empty split condition.
	* config/rs6000/vsx.md (*vsx_le_undo_permute_<mode>,
	vsx_reduc_<VEC_reduc_name>_v2df, vsx_reduc_<VEC_reduc_name>_v4sf,
	*vsx_reduc_<VEC_reduc_name>_v2df_scalar,
	*vsx_reduc_<VEC_reduc_name>_v4sf_scalar): Likewise.

Hi!

On Fri, Mar 19, 2021 at 10:46:54AM +0800, Kewen.Lin wrote:
> As Segher and Mike pointed out, the define_insn_and_split should avoid
> to use empty split condition if the condition for define_insn isn't empty,
> otherwise it can sometimes leads to unexpected consequence.
> 
> This patch is to fix some places like this.
> 
> Bootstrapped/regtested on powerpc64le-linux-gnu P9 and
> powerpc64-linux-gnu P8.
> 
> Since it's in very low risk and can avoid some unexpected errors,
> is it ok for trunk? or has to be for GCC12?

I am curious if the splitters ever triggered where they should not have?

> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 592ac90aa44..6ab71979566 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -4281,7 +4281,7 @@

If you add

[diff "md"]
        xfuncname = "^\\(define.*$"

to your .git/config, the patch will show what insn this is in:

> @@ -4281,7 +4281,7 @@ (define_insn_and_split "*rotldi3_insert_sf"


The patch is okay for trunk.  Thank you!

You might want to make this error easier to detect?  Maybe make
define_insn_and_split raise an error if the split condition is empty but
the insn condition is not?


Segher

Hi Segher,

Thanks for the review.

on 2021/3/19 下午8:36, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Mar 19, 2021 at 10:46:54AM +0800, Kewen.Lin wrote:
>> As Segher and Mike pointed out, the define_insn_and_split should avoid
>> to use empty split condition if the condition for define_insn isn't empty,
>> otherwise it can sometimes leads to unexpected consequence.
>>
>> This patch is to fix some places like this.
>>
>> Bootstrapped/regtested on powerpc64le-linux-gnu P9 and
>> powerpc64-linux-gnu P8.
>>
>> Since it's in very low risk and can avoid some unexpected errors,
>> is it ok for trunk? or has to be for GCC12?
> 
> I am curious if the splitters ever triggered where they should not have?
> 

Do you have any suggestion to catch this?  I thought the regression
testing probably can show something different but it didn't unfortunately.

>> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
>> index 592ac90aa44..6ab71979566 100644
>> --- a/gcc/config/rs6000/rs6000.md
>> +++ b/gcc/config/rs6000/rs6000.md
>> @@ -4281,7 +4281,7 @@
> 
> If you add
> 
> [diff "md"]
>         xfuncname = "^\\(define.*$"
> 
> to your .git/config, the patch will show what insn this is in:
> 

Thanks for the tips!

>> @@ -4281,7 +4281,7 @@ (define_insn_and_split "*rotldi3_insert_sf"
> 
> 
> The patch is okay for trunk.  Thank you!
> 
> You might want to make this error easier to detect?  Maybe make
> define_insn_and_split raise an error if the split condition is empty but
> the insn condition is not?
> 


Good idea!  Is there any possible reason to write this kind of
define_insn_and_split?  If no (we should forbid it), I think we can add
the check and raise an error if hits in gensupport.c.

I'll send out a RFC once GCC12 starts.


BR,
Kewen

On Fri, Mar 19, 2021 at 11:50:41PM +0800, Kewen.Lin wrote:
> > I am curious if the splitters ever triggered where they should not have?
> 
> Do you have any suggestion to catch this?  I thought the regression
> testing probably can show something different but it didn't unfortunately.

Well, you can compare the generated binaries with a compiler binary
before and after the patch, for something that will match the splitters,
so with a lot of float<->integer conversions for example.

I usually look at cc1 or vmlinux, but those both are nice for integer
code only.

Maybe we should have some tool that for every define_insn finds which
define_splits can possibly match it.

> > You might want to make this error easier to detect?  Maybe make
> > define_insn_and_split raise an error if the split condition is empty but
> > the insn condition is not?
> 
> Good idea!  Is there any possible reason to write this kind of
> define_insn_and_split?  If no (we should forbid it), I think we can add
> the check and raise an error if hits in gensupport.c.
> 
> I'll send out a RFC once GCC12 starts.

There is no good reason for it.  If you really want a
define_insn_and_split that splits more than just that insn, you should
just write the define_insn and the define_split separately.  Much
clearer that way.  A reader does not expect a define_insn_and_split to
split any other insns.  (Or they should not, IMO, of course :-) )

And yes, because that might well break things for some targets, or
change behaviour at least, it is a GCC 12 thing.  It will be a nice
cleanup though :-)


Segher

Hi Segher,

on 2021/3/20 上午5:58, Segher Boessenkool wrote:
> On Fri, Mar 19, 2021 at 11:50:41PM +0800, Kewen.Lin wrote:
>>> I am curious if the splitters ever triggered where they should not have?
>>
>> Do you have any suggestion to catch this?  I thought the regression
>> testing probably can show something different but it didn't unfortunately.
> 
> Well, you can compare the generated binaries with a compiler binary
> before and after the patch, for something that will match the splitters,
> so with a lot of float<->integer conversions for example.
> 
> I usually look at cc1 or vmlinux, but those both are nice for integer
> code only.
> 
> Maybe we should have some tool that for every define_insn finds which
> define_splits can possibly match it.
> 

Thanks for the suggestion.  I did the binary comparison for SPEC2017
executables built before and after this patch on both Power8 and Power9,
I still didn't see any differences, it seems hard to trigger it.

Committed via r11-7756.

>>> You might want to make this error easier to detect?  Maybe make
>>> define_insn_and_split raise an error if the split condition is empty but
>>> the insn condition is not?
>>
>> Good idea!  Is there any possible reason to write this kind of
>> define_insn_and_split?  If no (we should forbid it), I think we can add
>> the check and raise an error if hits in gensupport.c.
>>
>> I'll send out a RFC once GCC12 starts.
> 
> There is no good reason for it.  If you really want a
> define_insn_and_split that splits more than just that insn, you should
> just write the define_insn and the define_split separately.  Much
> clearer that way.  A reader does not expect a define_insn_and_split to
> split any other insns.  (Or they should not, IMO, of course :-) )
> 
> And yes, because that might well break things for some targets, or
> change behaviour at least, it is a GCC 12 thing.  It will be a nice
> cleanup though :-)
> 

Got it!  Thanks for the further explanation.  :) 


BR,
Kewen