[RFC/PATCH,00/11] Fix up some unexpected empty split conditions

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

Message

Richard Biener via Gcc-patches June 2, 2021, 5:04 a.m.
Hi all,

define_insn_and_split should avoid to use empty split condition
if the condition for define_insn isn't empty, otherwise it can
sometimes result in unexpected consequence, since the split
will always be done even if the insn condition doesn't hold.

To avoid forgetting to add "&& 1" onto split condition, as
Segher suggested in thread[1], this series is to add the check
and raise an error if it catches the unexpected cases.  With
this new check, we have to fix up some existing
define_insn_and_split which are detected as error.  I hope all
these places are not intentional to be kept as blank.

Any comments are highly appreciated.

BR,
Kewen

[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566970.html

Kewen Lin (11):
  gen: Emit error msg for empty split condition
  arc: Update unexpected empty split condition
  arm: Update unexpected empty split condition
  cris: Update unexpected empty split condition
  h8300: Update unexpected empty split condition
  i386: Update unexpected empty split condition
  m68k: Update unexpected empty split condition
  mips: Update unexpected empty split condition
  or1k: Update unexpected empty split condition
  sh: Update unexpected empty split condition
  sparc: Update unexpected empty split condition

 gcc/config/arc/arc.md        |  2 +-
 gcc/config/arm/vfp.md        |  4 ++--
 gcc/config/cris/cris.md      |  2 +-
 gcc/config/h8300/combiner.md |  2 +-
 gcc/config/i386/i386.md      |  6 +++---
 gcc/config/i386/sse.md       |  8 ++++----
 gcc/config/m68k/m68k.md      |  6 +++---
 gcc/config/mips/mips.md      |  6 +++---
 gcc/config/or1k/or1k.md      |  2 +-
 gcc/config/sh/sh.md          |  2 +-
 gcc/config/sparc/sparc.md    | 12 ++++++------
 gcc/gensupport.c             |  3 +++
 12 files changed, 29 insertions(+), 26 deletions(-)

-- 
2.17.1

Comments

Richard Biener via Gcc-patches June 2, 2021, 8:11 a.m. | #1
Kewen Lin <linkw@linux.ibm.com> writes:
> Hi all,

>

> define_insn_and_split should avoid to use empty split condition

> if the condition for define_insn isn't empty, otherwise it can

> sometimes result in unexpected consequence, since the split

> will always be done even if the insn condition doesn't hold.

>

> To avoid forgetting to add "&& 1" onto split condition, as

> Segher suggested in thread[1], this series is to add the check

> and raise an error if it catches the unexpected cases.  With

> this new check, we have to fix up some existing

> define_insn_and_split which are detected as error.  I hope all

> these places are not intentional to be kept as blank.


I wonder whether we should instead redefine the semantics of
define_insn_and_split so that the split condition is always applied
on top of the insn condition.  It's rare for a define_insn_and_split
to have independent insn and split conditions, so at the moment,
we're making the common case hard.

Thanks,
Richard

> Any comments are highly appreciated.

>

> BR,

> Kewen

>

> [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566970.html

>

> Kewen Lin (11):

>   gen: Emit error msg for empty split condition

>   arc: Update unexpected empty split condition

>   arm: Update unexpected empty split condition

>   cris: Update unexpected empty split condition

>   h8300: Update unexpected empty split condition

>   i386: Update unexpected empty split condition

>   m68k: Update unexpected empty split condition

>   mips: Update unexpected empty split condition

>   or1k: Update unexpected empty split condition

>   sh: Update unexpected empty split condition

>   sparc: Update unexpected empty split condition

>

>  gcc/config/arc/arc.md        |  2 +-

>  gcc/config/arm/vfp.md        |  4 ++--

>  gcc/config/cris/cris.md      |  2 +-

>  gcc/config/h8300/combiner.md |  2 +-

>  gcc/config/i386/i386.md      |  6 +++---

>  gcc/config/i386/sse.md       |  8 ++++----

>  gcc/config/m68k/m68k.md      |  6 +++---

>  gcc/config/mips/mips.md      |  6 +++---

>  gcc/config/or1k/or1k.md      |  2 +-

>  gcc/config/sh/sh.md          |  2 +-

>  gcc/config/sparc/sparc.md    | 12 ++++++------

>  gcc/gensupport.c             |  3 +++

>  12 files changed, 29 insertions(+), 26 deletions(-)
Richard Biener via Gcc-patches June 2, 2021, 8:37 a.m. | #2
Hi Richard,

on 2021/6/2 下午4:11, Richard Sandiford wrote:
> Kewen Lin <linkw@linux.ibm.com> writes:

>> Hi all,

>>

>> define_insn_and_split should avoid to use empty split condition

>> if the condition for define_insn isn't empty, otherwise it can

>> sometimes result in unexpected consequence, since the split

>> will always be done even if the insn condition doesn't hold.

>>

>> To avoid forgetting to add "&& 1" onto split condition, as

>> Segher suggested in thread[1], this series is to add the check

>> and raise an error if it catches the unexpected cases.  With

>> this new check, we have to fix up some existing

>> define_insn_and_split which are detected as error.  I hope all

>> these places are not intentional to be kept as blank.

> 

> I wonder whether we should instead redefine the semantics of

> define_insn_and_split so that the split condition is always applied

> on top of the insn condition.  It's rare for a define_insn_and_split

> to have independent insn and split conditions, so at the moment,

> we're making the common case hard.

> 


Just want to confirm that the suggestion is just applied for empty
split condition or all split conditions in define_insn_and_split? 
I guess it's the former?  It's like what Richi suggested with
auto-filling, the semantics redefining will eliminate the over-fill
concern.  Thanks for the suggestion!

BR,
Kewen
Richard Biener via Gcc-patches June 2, 2021, 9:13 a.m. | #3
"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi Richard,

>

> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:

>> Kewen Lin <linkw@linux.ibm.com> writes:

>>> Hi all,

>>>

>>> define_insn_and_split should avoid to use empty split condition

>>> if the condition for define_insn isn't empty, otherwise it can

>>> sometimes result in unexpected consequence, since the split

>>> will always be done even if the insn condition doesn't hold.

>>>

>>> To avoid forgetting to add "&& 1" onto split condition, as

>>> Segher suggested in thread[1], this series is to add the check

>>> and raise an error if it catches the unexpected cases.  With

>>> this new check, we have to fix up some existing

>>> define_insn_and_split which are detected as error.  I hope all

>>> these places are not intentional to be kept as blank.

>> 

>> I wonder whether we should instead redefine the semantics of

>> define_insn_and_split so that the split condition is always applied

>> on top of the insn condition.  It's rare for a define_insn_and_split

>> to have independent insn and split conditions, so at the moment,

>> we're making the common case hard.

>> 

>

> Just want to confirm that the suggestion is just applied for empty

> split condition or all split conditions in define_insn_and_split? 

> I guess it's the former?


No, I meant tha latter.  E.g. in:

(define_insn_and_split
  […]
  "TARGET_FOO"
  "…"
  […]
  "reload_completed"
  […]
)

the "reload_completed" condition is almost always a typo for
"&& reload_completed".

Like I say, it rarely makes sense for the split condition to
ignore the insn condition and specify an entirely independent condition.
There might be some define_insn_and_splits that do that, but it'd often
be less confusing to write the insn and split separately if so.

Even if we do want to support independent insn and split conditions,
that's always going to be the rare and surprising case, so it's the one
that should need extra syntax.

Thanks,
Richard

  It's like what Richi suggested with
> auto-filling, the semantics redefining will eliminate the over-fill

> concern.  Thanks for the suggestion!

>

> BR,

> Kewen
Richard Biener via Gcc-patches June 2, 2021, 10:01 a.m. | #4
on 2021/6/2 下午5:13, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:

>> Hi Richard,

>>

>> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:

>>> Kewen Lin <linkw@linux.ibm.com> writes:

>>>> Hi all,

>>>>

>>>> define_insn_and_split should avoid to use empty split condition

>>>> if the condition for define_insn isn't empty, otherwise it can

>>>> sometimes result in unexpected consequence, since the split

>>>> will always be done even if the insn condition doesn't hold.

>>>>

>>>> To avoid forgetting to add "&& 1" onto split condition, as

>>>> Segher suggested in thread[1], this series is to add the check

>>>> and raise an error if it catches the unexpected cases.  With

>>>> this new check, we have to fix up some existing

>>>> define_insn_and_split which are detected as error.  I hope all

>>>> these places are not intentional to be kept as blank.

>>>

>>> I wonder whether we should instead redefine the semantics of

>>> define_insn_and_split so that the split condition is always applied

>>> on top of the insn condition.  It's rare for a define_insn_and_split

>>> to have independent insn and split conditions, so at the moment,

>>> we're making the common case hard.

>>>

>>

>> Just want to confirm that the suggestion is just applied for empty

>> split condition or all split conditions in define_insn_and_split? 

>> I guess it's the former?

> 

> No, I meant tha latter.  E.g. in:

> 

> (define_insn_and_split

>   […]

>   "TARGET_FOO"

>   "…"

>   […]

>   "reload_completed"

>   […]

> )

> 

> the "reload_completed" condition is almost always a typo for

> "&& reload_completed".

> 

> Like I say, it rarely makes sense for the split condition to

> ignore the insn condition and specify an entirely independent condition.

> There might be some define_insn_and_splits that do that, but it'd often

> be less confusing to write the insn and split separately if so.

> 

> Even if we do want to support independent insn and split conditions,

> that's always going to be the rare and surprising case, so it's the one

> that should need extra syntax.

> 


Thanks for the clarification!

Since it may impact all ports, I wonder if there is a way to find out
this kind of "rare and surprising" case without a big coverage testing?
I'm happy to make a draft patch for it, but not sure how to early catch
those cases which need to be rewritten for those ports that I can't test
on (even with cfarm machines, the coverage seems still limited).

BR,
Kewen
Richard Biener via Gcc-patches June 2, 2021, 10:12 a.m. | #5
On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>

> on 2021/6/2 下午5:13, Richard Sandiford wrote:

> > "Kewen.Lin" <linkw@linux.ibm.com> writes:

> >> Hi Richard,

> >>

> >> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:

> >>> Kewen Lin <linkw@linux.ibm.com> writes:

> >>>> Hi all,

> >>>>

> >>>> define_insn_and_split should avoid to use empty split condition

> >>>> if the condition for define_insn isn't empty, otherwise it can

> >>>> sometimes result in unexpected consequence, since the split

> >>>> will always be done even if the insn condition doesn't hold.

> >>>>

> >>>> To avoid forgetting to add "&& 1" onto split condition, as

> >>>> Segher suggested in thread[1], this series is to add the check

> >>>> and raise an error if it catches the unexpected cases.  With

> >>>> this new check, we have to fix up some existing

> >>>> define_insn_and_split which are detected as error.  I hope all

> >>>> these places are not intentional to be kept as blank.

> >>>

> >>> I wonder whether we should instead redefine the semantics of

> >>> define_insn_and_split so that the split condition is always applied

> >>> on top of the insn condition.  It's rare for a define_insn_and_split

> >>> to have independent insn and split conditions, so at the moment,

> >>> we're making the common case hard.

> >>>

> >>

> >> Just want to confirm that the suggestion is just applied for empty

> >> split condition or all split conditions in define_insn_and_split?

> >> I guess it's the former?

> >

> > No, I meant tha latter.  E.g. in:

> >

> > (define_insn_and_split

> >   […]

> >   "TARGET_FOO"

> >   "…"

> >   […]

> >   "reload_completed"

> >   […]

> > )

> >

> > the "reload_completed" condition is almost always a typo for

> > "&& reload_completed".

> >

> > Like I say, it rarely makes sense for the split condition to

> > ignore the insn condition and specify an entirely independent condition.

> > There might be some define_insn_and_splits that do that, but it'd often

> > be less confusing to write the insn and split separately if so.

> >

> > Even if we do want to support independent insn and split conditions,

> > that's always going to be the rare and surprising case, so it's the one

> > that should need extra syntax.

> >

>

> Thanks for the clarification!

>

> Since it may impact all ports, I wonder if there is a way to find out

> this kind of "rare and surprising" case without a big coverage testing?

> I'm happy to make a draft patch for it, but not sure how to early catch

> those cases which need to be rewritten for those ports that I can't test

> on (even with cfarm machines, the coverage seems still limited).


So what Richard suggests would be to disallow split conditions
that do not start with "&& ", it's probably easy to do that as well
and look for build fails.  That should catch all cases to look at.

Richard.

> BR,

> Kewen

>
Richard Biener via Gcc-patches June 2, 2021, 5:32 p.m. | #6
Richard Biener <richard.guenther@gmail.com> writes:
> On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote:

>>

>> on 2021/6/2 下午5:13, Richard Sandiford wrote:

>> > "Kewen.Lin" <linkw@linux.ibm.com> writes:

>> >> Hi Richard,

>> >>

>> >> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:

>> >>> Kewen Lin <linkw@linux.ibm.com> writes:

>> >>>> Hi all,

>> >>>>

>> >>>> define_insn_and_split should avoid to use empty split condition

>> >>>> if the condition for define_insn isn't empty, otherwise it can

>> >>>> sometimes result in unexpected consequence, since the split

>> >>>> will always be done even if the insn condition doesn't hold.

>> >>>>

>> >>>> To avoid forgetting to add "&& 1" onto split condition, as

>> >>>> Segher suggested in thread[1], this series is to add the check

>> >>>> and raise an error if it catches the unexpected cases.  With

>> >>>> this new check, we have to fix up some existing

>> >>>> define_insn_and_split which are detected as error.  I hope all

>> >>>> these places are not intentional to be kept as blank.

>> >>>

>> >>> I wonder whether we should instead redefine the semantics of

>> >>> define_insn_and_split so that the split condition is always applied

>> >>> on top of the insn condition.  It's rare for a define_insn_and_split

>> >>> to have independent insn and split conditions, so at the moment,

>> >>> we're making the common case hard.

>> >>>

>> >>

>> >> Just want to confirm that the suggestion is just applied for empty

>> >> split condition or all split conditions in define_insn_and_split?

>> >> I guess it's the former?

>> >

>> > No, I meant tha latter.  E.g. in:

>> >

>> > (define_insn_and_split

>> >   […]

>> >   "TARGET_FOO"

>> >   "…"

>> >   […]

>> >   "reload_completed"

>> >   […]

>> > )

>> >

>> > the "reload_completed" condition is almost always a typo for

>> > "&& reload_completed".

>> >

>> > Like I say, it rarely makes sense for the split condition to

>> > ignore the insn condition and specify an entirely independent condition.

>> > There might be some define_insn_and_splits that do that, but it'd often

>> > be less confusing to write the insn and split separately if so.

>> >

>> > Even if we do want to support independent insn and split conditions,

>> > that's always going to be the rare and surprising case, so it's the one

>> > that should need extra syntax.

>> >

>>

>> Thanks for the clarification!

>>

>> Since it may impact all ports, I wonder if there is a way to find out

>> this kind of "rare and surprising" case without a big coverage testing?

>> I'm happy to make a draft patch for it, but not sure how to early catch

>> those cases which need to be rewritten for those ports that I can't test

>> on (even with cfarm machines, the coverage seems still limited).

>

> So what Richard suggests would be to disallow split conditions

> that do not start with "&& ", it's probably easy to do that as well

> and look for build fails.  That should catch all cases to look at.


Yeah.  As a strawman proposal, how about:

- add a new "define_independent_insn_and_split" that has the
  current semantics of define_insn_and_split.  This should be
  mechanical.

- find the define_insn_and_splits that are missing the "&&", and where
  missing the "&&" might make a difference.  Change them to
  define_independent_insn_and_splits.

  Like Richard says, this can be done by temporarily disallowing
  define_insn_and_splits that have no "&&".

  I think this should remain a mechanical step.  If port maintainers
  think that the missing "&&" is a mistake, they should fix it as
  a separate patch.

- flip the default for define_insn_and_split so that the "&&" is implicit
  (but can still be specified redundantly)

Then port maintainers who don't mind the churn can remove the
redundant "&&"s from the remaining define_insn_and_splits.

Thanks,
Richard
Richard Biener via Gcc-patches June 2, 2021, 6:25 p.m. | #7
On 6/2/2021 11:32 AM, Richard Sandiford wrote:
> Richard Biener <richard.guenther@gmail.com> writes:

>> On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote:

>>> on 2021/6/2 下午5:13, Richard Sandiford wrote:

>>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:

>>>>> Hi Richard,

>>>>>

>>>>> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:

>>>>>> Kewen Lin <linkw@linux.ibm.com> writes:

>>>>>>> Hi all,

>>>>>>>

>>>>>>> define_insn_and_split should avoid to use empty split condition

>>>>>>> if the condition for define_insn isn't empty, otherwise it can

>>>>>>> sometimes result in unexpected consequence, since the split

>>>>>>> will always be done even if the insn condition doesn't hold.

>>>>>>>

>>>>>>> To avoid forgetting to add "&& 1" onto split condition, as

>>>>>>> Segher suggested in thread[1], this series is to add the check

>>>>>>> and raise an error if it catches the unexpected cases.  With

>>>>>>> this new check, we have to fix up some existing

>>>>>>> define_insn_and_split which are detected as error.  I hope all

>>>>>>> these places are not intentional to be kept as blank.

>>>>>> I wonder whether we should instead redefine the semantics of

>>>>>> define_insn_and_split so that the split condition is always applied

>>>>>> on top of the insn condition.  It's rare for a define_insn_and_split

>>>>>> to have independent insn and split conditions, so at the moment,

>>>>>> we're making the common case hard.

>>>>>>

>>>>> Just want to confirm that the suggestion is just applied for empty

>>>>> split condition or all split conditions in define_insn_and_split?

>>>>> I guess it's the former?

>>>> No, I meant tha latter.  E.g. in:

>>>>

>>>> (define_insn_and_split

>>>>    […]

>>>>    "TARGET_FOO"

>>>>    "…"

>>>>    […]

>>>>    "reload_completed"

>>>>    […]

>>>> )

>>>>

>>>> the "reload_completed" condition is almost always a typo for

>>>> "&& reload_completed".

>>>>

>>>> Like I say, it rarely makes sense for the split condition to

>>>> ignore the insn condition and specify an entirely independent condition.

>>>> There might be some define_insn_and_splits that do that, but it'd often

>>>> be less confusing to write the insn and split separately if so.

>>>>

>>>> Even if we do want to support independent insn and split conditions,

>>>> that's always going to be the rare and surprising case, so it's the one

>>>> that should need extra syntax.

>>>>

>>> Thanks for the clarification!

>>>

>>> Since it may impact all ports, I wonder if there is a way to find out

>>> this kind of "rare and surprising" case without a big coverage testing?

>>> I'm happy to make a draft patch for it, but not sure how to early catch

>>> those cases which need to be rewritten for those ports that I can't test

>>> on (even with cfarm machines, the coverage seems still limited).

>> So what Richard suggests would be to disallow split conditions

>> that do not start with "&& ", it's probably easy to do that as well

>> and look for build fails.  That should catch all cases to look at.

> Yeah.  As a strawman proposal, how about:

>

> - add a new "define_independent_insn_and_split" that has the

>    current semantics of define_insn_and_split.  This should be

>    mechanical.

>

> - find the define_insn_and_splits that are missing the "&&", and where

>    missing the "&&" might make a difference.  Change them to

>    define_independent_insn_and_splits.

>

>    Like Richard says, this can be done by temporarily disallowing

>    define_insn_and_splits that have no "&&".

>

>    I think this should remain a mechanical step.  If port maintainers

>    think that the missing "&&" is a mistake, they should fix it as

>    a separate patch.

>

> - flip the default for define_insn_and_split so that the "&&" is implicit

>    (but can still be specified redundantly)

>

> Then port maintainers who don't mind the churn can remove the

> redundant "&&"s from the remaining define_insn_and_splits.

That works for me.  If we'd had this in place earlier I wouldn't have 
mucked up the H8 port.
jeff
Segher Boessenkool June 2, 2021, 11:52 p.m. | #8
On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:
> Richard Biener <richard.guenther@gmail.com> writes:

> > So what Richard suggests would be to disallow split conditions

> > that do not start with "&& ", it's probably easy to do that as well

> > and look for build fails.  That should catch all cases to look at.

> 

> Yeah.  As a strawman proposal, how about:

> 

> - add a new "define_independent_insn_and_split" that has the

>   current semantics of define_insn_and_split.  This should be

>   mechanical.


I'd rather not have that -- we can just write separate define_insn and
define_split in that case.

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.

> - find the define_insn_and_splits that are missing the "&&", and where

>   missing the "&&" might make a difference.  Change them to

>   define_independent_insn_and_splits.

> 

>   Like Richard says, this can be done by temporarily disallowing

>   define_insn_and_splits that have no "&&".


If we make that change permanently, that is all steps we ever need!

Very old backends use the same insn condition and split condition
sometimes still; it isn't hard to detect that as well, if that seems
prudent.


Segher
Richard Biener via Gcc-patches June 3, 2021, 5:22 a.m. | #9
Hi Richi/Richard/Jeff/Segher,

Thanks for the comments!

on 2021/6/3 上午7:52, Segher Boessenkool wrote:
> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:

>> Richard Biener <richard.guenther@gmail.com> writes:

>>> So what Richard suggests would be to disallow split conditions

>>> that do not start with "&& ", it's probably easy to do that as well

>>> and look for build fails.  That should catch all cases to look at.

>>

>> Yeah.  As a strawman proposal, how about:

>>

>> - add a new "define_independent_insn_and_split" that has the

>>   current semantics of define_insn_and_split.  This should be

>>   mechanical.

> 

> I'd rather not have that -- we can just write separate define_insn and

> define_split in that case.

> 


Not sure if someone would argue that he/she would like to go with one shared
pattern as before, to avoid any possible differences between two seperated
patterns and have good maintainability (like only editing on place) and
slightly better efficiency.

> 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.

>> - find the define_insn_and_splits that are missing the "&&", and where

>>   missing the "&&" might make a difference.  Change them to

>>   define_independent_insn_and_splits.

>>

>>   Like Richard says, this can be done by temporarily disallowing

>>   define_insn_and_splits that have no "&&".

> 

> If we make that change permanently, that is all steps we ever need!

> 


So the question is that: whether we need to demand an explicit "&&".
Richard's proposal is for answer "no" which aligns with Richi's auto
filling advice before.  I think it would result in fewer changes since
those places without explicit "&&" are mostly unintentional, all the jobs
are done by implied "&&".  Its downside seems to be bad readability, new
readers may take it as two seperated conditions at first glance, but I
guess if we emphasize this change in the document it would be fine?
Or emitting one warning if missing an explicit "&&"?

BR,
Kewen
 
> Very old backends use the same insn condition and split condition

> sometimes still; it isn't hard to detect that as well, if that seems

> prudent.

> 

> 

> Segher

>
Segher Boessenkool June 3, 2021, 8 a.m. | #10
Hi!

On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote:
> on 2021/6/3 上午7:52, Segher Boessenkool wrote:

> >> - add a new "define_independent_insn_and_split" that has the

> >>   current semantics of define_insn_and_split.  This should be

> >>   mechanical.

> > 

> > I'd rather not have that -- we can just write separate define_insn and

> > define_split in that case.

> 

> Not sure if someone would argue that he/she would like to go with one shared

> pattern as before, to avoid any possible differences between two seperated

> patterns and have good maintainability (like only editing on place) and

> slightly better efficiency.


You only would do this if you have a different insn condition and split
condition, which is a very important thing to know, it doesn't hurt to
draw attention to it.  The efficiency is exactly the same btw,
define_insn_and_split is just syntactic sugar.

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

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


Segher
Richard Biener via Gcc-patches June 3, 2021, 8:05 a.m. | #11
"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi Richi/Richard/Jeff/Segher,

>

> Thanks for the comments!

>

> on 2021/6/3 锟斤拷锟斤拷7:52, Segher Boessenkool wrote:

>> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:

>>> Richard Biener <richard.guenther@gmail.com> writes:

>>>> So what Richard suggests would be to disallow split conditions

>>>> that do not start with "&& ", it's probably easy to do that as well

>>>> and look for build fails.  That should catch all cases to look at.

>>>

>>> Yeah.  As a strawman proposal, how about:

>>>

>>> - add a new "define_independent_insn_and_split" that has the

>>>   current semantics of define_insn_and_split.  This should be

>>>   mechanical.

>> 

>> I'd rather not have that -- we can just write separate define_insn and

>> define_split in that case.

>> 

>

> Not sure if someone would argue that he/she would like to go with one shared

> pattern as before, to avoid any possible differences between two seperated

> patterns and have good maintainability (like only editing on place) and

> slightly better efficiency.


Right.  Plus it creates less make-work.  If we didn't have it, someone
would need to split the define_insn_and_splits that don't currently
use "&&", then later someone might decide that the missing "&&" was a
mistake and need to put them together again (or just revert the patch
and edit from there, I guess).

Plus define_independent_insn_and_split would act as a flag for something
that might be suspect.  If we split them then the define_split condition
will seem to have been chosen deliberately in isolation.

>> 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.


I don't know.  "&& 1" looks kind of weird to me.

One thing I'd been wondering about a while ago was whether we should key
the split part of define_insn_and_splits off the insn code, instead of
repeating the pattern match and insn C condition.  That would make the
split apply only to the associated define_insns, whereas at the moment
they also apply to any earlier (less general) define_insn that happens
to match the pattern and the C conditions.  It would also reduce the
complexity of the autogenerated define_split logic.

I don't know whether that's a good idea or not.  But having an explicit
"&&" implies that the generator shouldn't do that, and that it should
retest the insn condition from scratch.

>>> - find the define_insn_and_splits that are missing the "&&", and where

>>>   missing the "&&" might make a difference.  Change them to

>>>   define_independent_insn_and_splits.

>>>

>>>   Like Richard says, this can be done by temporarily disallowing

>>>   define_insn_and_splits that have no "&&".

>> 

>> If we make that change permanently, that is all steps we ever need!

>> 

>

> So the question is that: whether we need to demand an explicit "&&".

> Richard's proposal is for answer "no" which aligns with Richi's auto

> filling advice before.  I think it would result in fewer changes since

> those places without explicit "&&" are mostly unintentional, all the jobs

> are done by implied "&&".  Its downside seems to be bad readability, new

> readers may take it as two seperated conditions at first glance, but I

> guess if we emphasize this change in the document it would be fine?

> Or emitting one warning if missing an explicit "&&"?


IMO the natural way to read it is that the split C condition gives the
conditions under which the instruction should be split.  I think that's
why forgetting the "&&" is such a common mistake.  (I know I've done it
plenty of times.)

IMO requiring the "&&" is baking in an alternative, less intuitive,
interpretation.

Thanks,
Richard
Segher Boessenkool June 3, 2021, 9:18 a.m. | #12
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 :-)

> 

> > > 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.


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));
Segher Boessenkool June 3, 2021, 10:01 a.m. | #13
On Thu, Jun 03, 2021 at 09:05:02AM +0100, Richard Sandiford via Gcc-patches wrote:
> Right.  Plus it creates less make-work.  If we didn't have it, someone

> would need to split the define_insn_and_splits that don't currently

> use "&&", then later someone might decide that the missing "&&" was a

> mistake and need to put them together again (or just revert the patch

> and edit from there, I guess).


I would hope no maintainer would be foolish enough to flip-flop like
that.

> Plus define_independent_insn_and_split would act as a flag for something

> that might be suspect.


That always is a conceptual error, even.  The name says so already: the
insn and the split are very different things, with different conditions,
they just happen to share a pattern.

> If we split them then the define_split condition

> will seem to have been chosen deliberately in isolation.


And it will have beenm chosen deliberately!  Why *else* write things
like this?

> > 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.

> 

> I don't know.  "&& 1" looks kind of weird to me.


We have it in rs6000.md since 2004.  sparc has had it since 2002.  i386
has had it since 2001.  arm still does not have it :-)

> > So the question is that: whether we need to demand an explicit "&&".

> > Richard's proposal is for answer "no" which aligns with Richi's auto

> > filling advice before.  I think it would result in fewer changes since

> > those places without explicit "&&" are mostly unintentional, all the jobs

> > are done by implied "&&".  Its downside seems to be bad readability, new

> > readers may take it as two seperated conditions at first glance, but I

> > guess if we emphasize this change in the document it would be fine?

> > Or emitting one warning if missing an explicit "&&"?

> 

> IMO the natural way to read it is that the split C condition gives the

> conditions under which the instruction should be split.  I think that's

> why forgetting the "&&" is such a common mistake.  (I know I've done it

> plenty of times.)


You even do in this description!  :-)  You do not want the define_split
in a define_insn_and_split to happen for insns that do not match the
insn condition, that would not be intuitive at all.

> IMO requiring the "&&" is baking in an alternative, less intuitive,

> interpretation.


But you want the exact same semantics, right?  I do agree that the
syntax without "&&" looks nicer.  It has many practical problems though,
so it is nice to aim for, but we cannot move there until we have all
existing machine descriptions fixed up first.


Segher
Richard Biener via Gcc-patches June 3, 2021, 10:25 a.m. | #14
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Jun 03, 2021 at 09:05:02AM +0100, Richard Sandiford via Gcc-patches wrote:

>> Right.  Plus it creates less make-work.  If we didn't have it, someone

>> would need to split the define_insn_and_splits that don't currently

>> use "&&", then later someone might decide that the missing "&&" was a

>> mistake and need to put them together again (or just revert the patch

>> and edit from there, I guess).

>

> I would hope no maintainer would be foolish enough to flip-flop like

> that.


But the point is that the maintainers wouldn't be flip-flopping.
The move to define_independent_insn_and_split would be a mechnical
change made by someone else.

We shouldn't just add "&&" to all define_insn_and_splits that currently
lack them.  That might well break things.  So to get the perfect result,
someone has to look at each individual define_insn_and_split that currently
lacks a "&&" to see what the effects of adding "&&" would be.

IMO it's not reasonable to ask Kewen to do that for all ports.  So the
process I suggested was a way of mechanically changing existing ports in
a way that would not require input from target maintainers, or extensive
testing on affected targets.  It would keep the behaviour of the port
the same as it is now (whether that's good or bad).  This part of the
process could be scripted.

>> > 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.

>> 

>> I don't know.  "&& 1" looks kind of weird to me.

>

> We have it in rs6000.md since 2004.  sparc has had it since 2002.  i386

> has had it since 2001.  arm still does not have it :-)


Sure, this syntax goes back 20 years.  I don't think that answers the
point though.  The question was whether a split condition "&& 1" is
more readable than a condition "", given that "" means "always" in other
contexts.  Given the choice, IMO "" is more readable and "&& 1" looks
weird/inconsistent.

Thanks,
Richard
Richard Biener via Gcc-patches June 3, 2021, 5:11 p.m. | #15
On 6/3/2021 2:00 AM, Segher Boessenkool wrote:
> Hi!

>

> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote:

>> on 2021/6/3 上午7:52, Segher Boessenkool wrote:

>>>> - add a new "define_independent_insn_and_split" that has the

>>>>    current semantics of define_insn_and_split.  This should be

>>>>    mechanical.

>>> I'd rather not have that -- we can just write separate define_insn and

>>> define_split in that case.

>> Not sure if someone would argue that he/she would like to go with one shared

>> pattern as before, to avoid any possible differences between two seperated

>> patterns and have good maintainability (like only editing on place) and

>> slightly better efficiency.

> You only would do this if you have a different insn condition and split

> condition, which is a very important thing to know, it doesn't hurt to

> draw attention to it.  The efficiency is exactly the same btw,

> define_insn_and_split is just syntactic sugar.

>

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

Exactly.   While these empty conditions or those not starting with "&&" 
are technically valid, they're all suspicious from a port correctness 
standpoint, particularly if the main condition is non-empty.

Having made that mistake when converting the H8 away from CC0, I can say 
fairly confidently that if we had this in place a year ago that those 
mistakes would likely have been avoided.  Thankfully the H8 isn't a 
heavily used port and has limped along until I stumbled over the issue a 
week or so ago while polishing some improvements to the port.
Jeff
Segher Boessenkool June 3, 2021, 9:25 p.m. | #16
On Thu, Jun 03, 2021 at 11:25:49AM +0100, Richard Sandiford wrote:
> We shouldn't just add "&&" to all define_insn_and_splits that currently

> lack them.


My previous post shows that this *already* is required.

> IMO it's not reasonable to ask Kewen to do that for all ports.  So the

> process I suggested was a way of mechanically changing existing ports in

> a way that would not require input from target maintainers, or extensive

> testing on affected targets.


I fear it will end up as Yet Another unfinished transition this way :-(

> >> I don't know.  "&& 1" looks kind of weird to me.

> >

> > We have it in rs6000.md since 2004.  sparc has had it since 2002.  i386

> > has had it since 2001.  arm still does not have it :-)

> 

> Sure, this syntax goes back 20 years.  I don't think that answers the

> point though.  The question was whether a split condition "&& 1" is

> more readable than a condition "", given that "" means "always" in other

> contexts.  Given the choice, IMO "" is more readable and "&& 1" looks

> weird/inconsistent.


In most ports "&& 1" is all over the place and well-known already.  Sure
we could change to "" meaning always, but that is not what it currently
means!

If we could just start all over we could do it perfectly (but see
second-system syndrome, heh).  But we cannot.  IMO we should especially
avoid everything that uses new semantics for old syntax.


Segher
Richard Biener via Gcc-patches June 3, 2021, 9:34 p.m. | #17
On Thu, Jun 03, 2021 at 04:25:44PM -0500, Segher Boessenkool wrote:
> If we could just start all over we could do it perfectly (but see

> second-system syndrome, heh).  But we cannot.  IMO we should especially

> avoid everything that uses new semantics for old syntax.


Agreed, that would be a nightmare for backporting.

	Jakub
Segher Boessenkool June 3, 2021, 10:19 p.m. | #18
On Thu, Jun 03, 2021 at 11:11:53AM -0600, Jeff Law wrote:
> On 6/3/2021 2:00 AM, Segher Boessenkool 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 :-)

> Exactly.   While these empty conditions or those not starting with "&&" 

> are technically valid, they're all suspicious from a port correctness 

> standpoint, particularly if the main condition is non-empty.


And note that this is also the case if you wrote the insn condition as
an empty string, but you used some iterator with a condition.  I found
many of these in rs6000.  This will need to be fixed before we do
anything else.

> Having made that mistake when converting the H8 away from CC0, I can say 

> fairly confidently that if we had this in place a year ago that those 

> mistakes would likely have been avoided.  Thankfully the H8 isn't a 

> heavily used port and has limped along until I stumbled over the issue a 

> week or so ago while polishing some improvements to the port.


I've noticed unexpected splits in rs6000 only a few times over the
years. That doesn't mean more didn't happen, they just didn't cause
obvious enough problems :-)


Segher
Richard Biener via Gcc-patches June 4, 2021, 2:57 a.m. | #19
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));

>
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index abfd845..86869d9 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1283,7 +1283,7 @@ (define_insn_and_split "*movsi_aarch64"
    fmov\\t%w0, %s1
    fmov\\t%s0, %s1
    * return aarch64_output_scalar_simd_mov_immediate (operands[1], SImode);"
-  "CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), SImode)
+  "&& CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), SImode)
     && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))"
    [(const_int 0)]
    "{
@@ -1319,7 +1319,7 @@ (define_insn_and_split "*movdi_aarch64"
    fmov\\t%x0, %d1
    fmov\\t%d0, %d1
    * return aarch64_output_scalar_simd_mov_immediate (operands[1], DImode);"
-   "(CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), DImode))
+   "&& (CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), DImode))
     && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))"
    [(const_int 0)]
    "{
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 9ff35d9..8d72f34 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -5249,7 +5249,7 @@ (define_insn_and_split "*add<dwi>3_doubleword"
    (clobber (reg:CC FLAGS_REG))]
   "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CCC FLAGS_REG)
 		   (compare:CCC
 		     (plus:DWIH (match_dup 1) (match_dup 2))
@@ -6050,7 +6050,7 @@ (define_insn_and_split "*addv<dwi>4_doubleword"
 	(plus:<DWI> (match_dup 1) (match_dup 2)))]
   "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CCC FLAGS_REG)
 		   (compare:CCC
 		     (plus:DWIH (match_dup 1) (match_dup 2))
@@ -6097,7 +6097,7 @@ (define_insn_and_split "*addv<dwi>4_doubleword_1"
    && CONST_SCALAR_INT_P (operands[2])
    && rtx_equal_p (operands[2], operands[3])"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CCC FLAGS_REG)
 		   (compare:CCC
 		     (plus:DWIH (match_dup 1) (match_dup 2))
@@ -6391,7 +6391,7 @@ (define_insn_and_split "*sub<dwi>3_doubleword"
    (clobber (reg:CC FLAGS_REG))]
   "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CC FLAGS_REG)
 		   (compare:CC (match_dup 1) (match_dup 2)))
 	      (set (match_dup 0)
@@ -6559,7 +6559,7 @@ (define_insn_and_split "*subv<dwi>4_doubleword"
 	(minus:<DWI> (match_dup 1) (match_dup 2)))]
   "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CC FLAGS_REG)
 		   (compare:CC (match_dup 1) (match_dup 2)))
 	      (set (match_dup 0)
@@ -6604,7 +6604,7 @@ (define_insn_and_split "*subv<dwi>4_doubleword_1"
    && CONST_SCALAR_INT_P (operands[2])
    && rtx_equal_p (operands[2], operands[3])"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CC FLAGS_REG)
 		   (compare:CC (match_dup 1) (match_dup 2)))
 	      (set (match_dup 0)
@@ -7204,7 +7204,7 @@ (define_insn_and_split "*add<dwi>3_doubleword_cc_overflow_1"
 	(plus:<DWI> (match_dup 1) (match_dup 2)))]
   "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CCC FLAGS_REG)
 		   (compare:CCC
 		     (plus:DWIH (match_dup 1) (match_dup 2))
@@ -8161,7 +8161,7 @@ (define_insn_and_split "divmod<mode>4_1"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (match_dup 1)
 		   (ashiftrt:SWI48 (match_dup 4) (match_dup 5)))
 	      (clobber (reg:CC FLAGS_REG))])
@@ -8196,7 +8196,7 @@ (define_insn_and_split "udivmod<mode>4_1"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(set (match_dup 1) (const_int 0))
    (parallel [(set (match_dup 0)
 		   (udiv:SWI48 (match_dup 2) (match_dup 3)))
@@ -8336,7 +8336,7 @@ (define_insn_and_split "*divmod<mode>4"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (match_dup 1)
 		   (ashiftrt:SWIM248 (match_dup 4) (match_dup 5)))
 	      (clobber (reg:CC FLAGS_REG))])
@@ -8371,7 +8371,7 @@ (define_insn_and_split "*udivmod<mode>4"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(set (match_dup 1) (const_int 0))
    (parallel [(set (match_dup 0)
 		   (udiv:SWIM248 (match_dup 2) (match_dup 3)))
@@ -10069,7 +10069,7 @@ (define_insn_and_split "*neg<dwi>2_doubleword"
    (clobber (reg:CC FLAGS_REG))]
   "ix86_unary_operator_ok (NEG, <DWI>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel
     [(set (reg:CCC FLAGS_REG)
 	  (ne:CCC (match_dup 1) (const_int 0)))
@@ -11545,7 +11545,7 @@ (define_insn_and_split "*<insn><mode>3_doubleword"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "epilogue_completed"
+  "&& epilogue_completed"
   [(const_int 0)]
   "ix86_split_<insn> (operands, NULL_RTX, <MODE>mode); DONE;"
   [(set_attr "type" "multi")])
@@ -12045,7 +12045,7 @@ (define_insn_and_split "ix86_rotl<dwi>3_doubleword"
   (clobber (match_scratch:DWIH 3 "=&r"))]
  ""
  "#"
- "reload_completed"
+ "&& reload_completed"
  [(set (match_dup 3) (match_dup 4))
   (parallel
    [(set (match_dup 4)
@@ -12073,7 +12073,7 @@ (define_insn_and_split "ix86_rotr<dwi>3_doubleword"
   (clobber (match_scratch:DWIH 3 "=&r"))]
  ""
  "#"
- "reload_completed"
+ "&& reload_completed"
  [(set (match_dup 3) (match_dup 4))
   (parallel
    [(set (match_dup 4)
@@ -14308,7 +14308,7 @@ (define_insn_and_split "ctz<mode>2"
 
   return "bsf{<imodesuffix>}\t{%1, %0|%0, %1}";
 }
-  "(TARGET_BMI || TARGET_GENERIC)
+  "&& (TARGET_BMI || TARGET_GENERIC)
    && TARGET_AVOID_FALSE_DEP_FOR_BMI && epilogue_completed
    && optimize_function_for_speed_p (cfun)
    && !reg_mentioned_p (operands[0], operands[1])"
@@ -15712,7 +15712,7 @@ (define_insn_and_split "*load_tp_x32_zext"
 	  (unspec:SI [(const_int 0)] UNSPEC_TP)))]
   "TARGET_X32"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(zero_extend:DI (match_dup 1)))]
 {
@@ -15750,7 +15750,7 @@ (define_insn_and_split "*add_tp_x32_zext"
    (clobber (reg:CC FLAGS_REG))]
   "TARGET_X32"
   "#"
-  ""
+  "&& 1"
   [(parallel
      [(set (match_dup 0)
      	   (zero_extend:DI
@@ -15841,7 +15841,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_32"
    (clobber (reg:CC FLAGS_REG))]
   "!TARGET_64BIT && TARGET_GNU2_TLS"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0) (match_dup 5))]
 {
   operands[5] = can_create_pseudo_p () ? gen_reg_rtx (Pmode) : operands[0];
@@ -15900,7 +15900,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_64_<mode>"
    (clobber (reg:CC FLAGS_REG))]
   "TARGET_64BIT && TARGET_GNU2_TLS"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0) (match_dup 4))]
 {
   operands[4] = can_create_pseudo_p () ? gen_reg_rtx (ptr_mode) : operands[0];
diff --git a/gcc/config/i386/mmx.md b/gcc/config/i386/mmx.md
index 4c2b724..e6737b1 100644
--- a/gcc/config/i386/mmx.md
+++ b/gcc/config/i386/mmx.md
@@ -1668,7 +1668,7 @@ (define_insn_and_split "mmx_pack<s_trunsuffix>swb"
    pack<s_trunsuffix>swb\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_pack (operands, <any_s_truncate:CODE>); DONE;"
@@ -1688,7 +1688,7 @@ (define_insn_and_split "mmx_packssdw"
    packssdw\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_pack (operands, SS_TRUNCATE); DONE;"
@@ -1711,7 +1711,7 @@ (define_insn_and_split "mmx_punpckhbw"
    punpckhbw\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, true); DONE;"
@@ -1734,7 +1734,7 @@ (define_insn_and_split "mmx_punpcklbw"
    punpcklbw\t{%2, %0|%0, %k2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, false); DONE;"
@@ -1755,7 +1755,7 @@ (define_insn_and_split "mmx_punpckhwd"
    punpckhwd\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, true); DONE;"
@@ -1776,7 +1776,7 @@ (define_insn_and_split "mmx_punpcklwd"
    punpcklwd\t{%2, %0|%0, %k2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, false); DONE;"
@@ -1797,7 +1797,7 @@ (define_insn_and_split "mmx_punpckhdq"
    punpckhdq\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, true); DONE;"
@@ -1818,7 +1818,7 @@ (define_insn_and_split "mmx_punpckldq"
    punpckldq\t{%2, %0|%0, %k2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, false); DONE;"
@@ -2542,7 +2542,7 @@ (define_insn_and_split "mmx_pmovmskb"
   "@
    pmovmskb\t{%1, %0|%0, %1}
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[1]))"
   [(set (match_dup 0)
         (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))
diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
index 9d3728d..9919cc0 100644
--- a/gcc/config/i386/sse.md
+++ b/gcc/config/i386/sse.md
@@ -5223,7 +5223,7 @@ (define_insn_and_split "sse_cvtpi2ps"
    cvtpi2ps\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REG_P (operands[2])"
   [(const_int 0)]
 {
@@ -5278,7 +5278,7 @@ (define_insn_and_split "sse_cvtps2pi"
   "@
    cvtps2pi\t{%1, %0|%0, %q1}
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REG_P (operands[0])"
   [(const_int 0)]
 {
@@ -5310,7 +5310,7 @@ (define_insn_and_split "sse_cvttps2pi"
   "@
    cvttps2pi\t{%1, %0|%0, %q1}
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REG_P (operands[0])"
   [(const_int 0)]
 {
@@ -16467,7 +16467,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_lt"
 	  UNSPEC_MOVMSK))]
   "TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))]
   ""
@@ -16489,7 +16489,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_zext_lt"
 	    UNSPEC_MOVMSK)))]
   "TARGET_64BIT && TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(zero_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))]
   ""
@@ -16511,7 +16511,7 @@ (define_insn_and_split "*sse2_pmovmskb_ext_lt"
 	    UNSPEC_MOVMSK)))]
   "TARGET_64BIT && TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(sign_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))]
   ""
@@ -16692,7 +16692,7 @@ (define_insn_and_split "ssse3_ph<plusminus_mnemonic>wv4hi3"
    ph<plusminus_mnemonic>w\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSSE3 && reload_completed
+  "&& TARGET_SSSE3 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
 {
@@ -16777,7 +16777,7 @@ (define_insn_and_split "ssse3_ph<plusminus_mnemonic>dv2si3"
    ph<plusminus_mnemonic>d\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSSE3 && reload_completed
+  "&& TARGET_SSSE3 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
 {
@@ -17185,7 +17185,7 @@ (define_insn_and_split "*ssse3_pshufbv8qi3"
    pshufb\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSSE3 && reload_completed
+  "&& TARGET_SSSE3 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(set (match_dup 3)
 	(and:V4SI (match_dup 3) (match_dup 2)))
@@ -17315,7 +17315,7 @@ (define_insn_and_split "ssse3_palignrdi"
       gcc_unreachable ();
     }
 }
-  "TARGET_SSSE3 && reload_completed
+  "&& TARGET_SSSE3 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(set (match_dup 0)
 	(lshiftrt:V1TI (match_dup 0) (match_dup 3)))]
@@ -17769,7 +17769,7 @@ (define_insn_and_split "*<sse4_1_avx2>_pblendvb_lt"
 	  UNSPEC_BLENDV))]
   "TARGET_SSE4_1"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(unspec:VI1_AVX2
 	 [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPEC_BLENDV))]
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 3f59b544f6a..feeeaffcc35 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -6752,7 +6752,7 @@ (define_insn_and_split "*and<mode>3_internal"
 
   return "#";
 }
-  "reload_completed && int_reg_operand (operands[0], <MODE>mode)"
+  "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, AND, false, false, false);
@@ -6788,7 +6788,7 @@ (define_insn_and_split "*bool<mode>3_internal"
 
   return "#";
 }
-  "reload_completed && int_reg_operand (operands[0], <MODE>mode)"
+  "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, GET_CODE (operands[3]), false, false, false);
@@ -6825,7 +6825,7 @@ (define_insn_and_split "*boolc<mode>3_internal1"
 
   return "#";
 }
-  "(TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND))
+  "&& (TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND))
    && reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
@@ -6854,7 +6854,7 @@ (define_insn_and_split "*boolc<mode>3_internal2"
 	  (match_operand:TI2 1 "int_reg_operand" "r,r,0")]))]
   "!TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
   "#"
-  "reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
+  "&& reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, GET_CODE (operands[3]), false, false, true);
@@ -6885,7 +6885,7 @@ (define_insn_and_split "*boolcc<mode>3_internal1"
 
   return "#";
 }
-  "(TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND))
+  "&& (TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND))
    && reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
@@ -6915,7 +6915,7 @@ (define_insn_and_split "*boolcc<mode>3_internal2"
 	   (match_operand:TI2 2 "int_reg_operand" "r,r,0"))]))]
   "!TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
   "#"
-  "reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
+  "&& reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, GET_CODE (operands[3]), false, true, true);
@@ -6943,7 +6943,7 @@ (define_insn_and_split "*eqv<mode>3_internal1"
 
   return "#";
 }
-  "TARGET_P8_VECTOR && reload_completed
+  "&& TARGET_P8_VECTOR && reload_completed
    && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
@@ -6972,7 +6972,7 @@ (define_insn_and_split "*eqv<mode>3_internal2"
 	  (match_operand:TI2 2 "int_reg_operand" "r,r,0"))))]
   "!TARGET_P8_VECTOR"
   "#"
-  "reload_completed && !TARGET_P8_VECTOR"
+  "&& reload_completed && !TARGET_P8_VECTOR"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, XOR, true, false, false);
@@ -7000,7 +7000,7 @@ (define_insn_and_split "one_cmpl<mode>2"
 
   return "#";
 }
-  "reload_completed && int_reg_operand (operands[0], <MODE>mode)"
+  "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, NOT, false, false, false);
@@ -13532,7 +13532,7 @@ (define_insn_and_split "@eh_set_lr_<mode>"
    (clobber (match_scratch:P 1 "=&b"))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(const_int 0)]
 {
   rs6000_emit_eh_reg_restore (operands[0], operands[1]);
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index bcb92be2f5c..2e0abb27354 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -1630,7 +1630,7 @@ (define_insn_and_split "vsx_mul_v2di"
                      UNSPEC_VSX_MULSD))]
   "VECTOR_MEM_VSX_P (V2DImode)"
   "#"
-  "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
+  "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
   [(const_int 0)]
 {
   rtx op0 = operands[0];
@@ -1685,7 +1685,7 @@ (define_insn_and_split "vsx_div_v2di"
                      UNSPEC_VSX_DIVSD))]
   "VECTOR_MEM_VSX_P (V2DImode)"
   "#"
-  "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
+  "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
   [(const_int 0)]
 {
   rtx op0 = operands[0];
@@ -1732,7 +1732,7 @@ (define_insn_and_split "vsx_udiv_v2di"
                      UNSPEC_VSX_DIVUD))]
   "VECTOR_MEM_VSX_P (V2DImode)"
   "#"
-  "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
+  "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
   [(const_int 0)]
 {
   rtx op0 = operands[0];
Richard Biener via Gcc-patches June 4, 2021, 3:33 a.m. | #20
on 2021/6/3 下午4:05, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:

>> Hi Richi/Richard/Jeff/Segher,

>>

>> Thanks for the comments!

>>

>> on 2021/6/3 锟斤拷锟斤拷7:52, Segher Boessenkool wrote:

>>> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:

>>>> Richard Biener <richard.guenther@gmail.com> writes:

>>>>> So what Richard suggests would be to disallow split conditions

>>>>> that do not start with "&& ", it's probably easy to do that as well

>>>>> and look for build fails.  That should catch all cases to look at.

>>>>

>>>> Yeah.  As a strawman proposal, how about:

>>>>

>>>> - add a new "define_independent_insn_and_split" that has the

>>>>   current semantics of define_insn_and_split.  This should be

>>>>   mechanical.

>>>

>>> I'd rather not have that -- we can just write separate define_insn and

>>> define_split in that case.

>>>

>>

>> Not sure if someone would argue that he/she would like to go with one shared

>> pattern as before, to avoid any possible differences between two seperated

>> patterns and have good maintainability (like only editing on place) and

>> slightly better efficiency.

> 

> Right.  Plus it creates less make-work.  If we didn't have it, someone

> would need to split the define_insn_and_splits that don't currently

> use "&&", then later someone might decide that the missing "&&" was a

> mistake and need to put them together again (or just revert the patch

> and edit from there, I guess).

> 

> Plus define_independent_insn_and_split would act as a flag for something

> that might be suspect.  If we split them then the define_split condition

> will seem to have been chosen deliberately in isolation.

> 

>>> 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.

> 

> I don't know.  "&& 1" looks kind of weird to me.

> 

> One thing I'd been wondering about a while ago was whether we should key

> the split part of define_insn_and_splits off the insn code, instead of

> repeating the pattern match and insn C condition.  That would make the

> split apply only to the associated define_insns, whereas at the moment

> they also apply to any earlier (less general) define_insn that happens

> to match the pattern and the C conditions.  It would also reduce the

> complexity of the autogenerated define_split logic.

> 

> I don't know whether that's a good idea or not.  But having an explicit

> "&&" implies that the generator shouldn't do that, and that it should

> retest the insn condition from scratch.

> 

>>>> - find the define_insn_and_splits that are missing the "&&", and where

>>>>   missing the "&&" might make a difference.  Change them to

>>>>   define_independent_insn_and_splits.

>>>>

>>>>   Like Richard says, this can be done by temporarily disallowing

>>>>   define_insn_and_splits that have no "&&".

>>>

>>> If we make that change permanently, that is all steps we ever need!

>>>

>>

>> So the question is that: whether we need to demand an explicit "&&".

>> Richard's proposal is for answer "no" which aligns with Richi's auto

>> filling advice before.  I think it would result in fewer changes since

>> those places without explicit "&&" are mostly unintentional, all the jobs

>> are done by implied "&&".  Its downside seems to be bad readability, new

>> readers may take it as two seperated conditions at first glance, but I

>> guess if we emphasize this change in the document it would be fine?

>> Or emitting one warning if missing an explicit "&&"?

> 

> IMO the natural way to read it is that the split C condition gives the

> conditions under which the instruction should be split.  I think that's

> why forgetting the "&&" is such a common mistake.  (I know I've done it

> plenty of times.)

> 

> IMO requiring the "&&" is baking in an alternative, less intuitive,

> interpretation.

> 


Thanks for the explanation, I was thinking people may have got used to
the starting "&&" in split condition, so it's easy for them to read.
But I agree it's better not to have it in the natural way.

BR,
Kewen
Richard Biener via Gcc-patches June 7, 2021, 7:12 a.m. | #21
On Fri, Jun 4, 2021 at 4:58 AM Kewen.Lin via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>

> 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.


For preserving existing behavior with requiring "&& " we can (mechanically?)
split define_insn_and_split into define_insn and define_split with the old
condition, right?

That is, all empty insn condition cases are of course obvious to fix.

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

> >
Segher Boessenkool June 7, 2021, 11:50 p.m. | #22
Hi!

On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:
> 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.


https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html

This adds an "&&" everywhere (or in fact, it just skips any existing
one, it just has the same effect of adding it everywhere).  I tested it
with building gcc and Linux for all supported targets (31 of them; I do
some with multiple configs, mostly 32-bit and 64-bit).  None had any
difference before and after the change.

So I am no longer worried that there will be any fallout from doing
this.  There are many things that *could* go wrong, but I don't think
there will be enough at all to be an impediment to just throwing the
switch.

If we go this way no target will need any significant fixing, maybe none
at all will be needed across all targets.  And no changes will be needed
anywhere immediately.  We could make leading "&&" deprecated, and the
same for split condition "1" (which was "&& 1").  This is easy to change
automatically as well.


Segher
Richard Biener via Gcc-patches June 8, 2021, 1:45 a.m. | #23
on 2021/6/7 下午3:12, Richard Biener wrote:
> On Fri, Jun 4, 2021 at 4:58 AM Kewen.Lin via Gcc-patches

> <gcc-patches@gcc.gnu.org> wrote:

>>

>> 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.

> 

> For preserving existing behavior with requiring "&& " we can (mechanically?)

> split define_insn_and_split into define_insn and define_split with the old

> condition, right?

> 


Yeah, we can replace all possible "unsafe" ones with separated define_insn
and define_split.

> That is, all empty insn condition cases are of course obvious to fix.

> 


Sorry for the confusion, those ones are false positive.  Mode iterators
can append more conditions onto empty insn condition to make it not empty,
I noticed it happened for split condition at the same time.  The previous
checking is very rough and didn't care about them as they are not harmful
for the testing.  My latest local patch has excluded them by checking the
beginning and the end, also for the variant with one more external "()".

BR,
Kewen

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

>>>
Richard Biener via Gcc-patches June 8, 2021, 2:09 a.m. | #24
Hi Segher,

on 2021/6/8 上午7:50, Segher Boessenkool wrote:
> Hi!

> 

> On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:

>> 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.

> 

> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html

> 

> This adds an "&&" everywhere (or in fact, it just skips any existing

> one, it just has the same effect of adding it everywhere).  I tested it

> with building gcc and Linux for all supported targets (31 of them; I do

> some with multiple configs, mostly 32-bit and 64-bit).  None had any

> difference before and after the change.

> 

> So I am no longer worried that there will be any fallout from doing

> this.  There are many things that *could* go wrong, but I don't think

> there will be enough at all to be an impediment to just throwing the

> switch.

> 

> If we go this way no target will need any significant fixing, maybe none

> at all will be needed across all targets.  And no changes will be needed

> anywhere immediately.  We could make leading "&&" deprecated, and the

> same for split condition "1" (which was "&& 1").  This is easy to change

> automatically as well.

> 

Thanks very much for doing this!

I guess we are not going to backport this?  If we won't, it seems to need
some way to ensure the implied "&&" will show up explicitly when backporting
some define_insn_and_split.

BR,
Kewen
Richard Biener via Gcc-patches June 8, 2021, 7:08 a.m. | #25
On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>

> Hi Segher,

>

> on 2021/6/8 上午7:50, Segher Boessenkool wrote:

> > Hi!

> >

> > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:

> >> 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.

> >

> > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html

> >

> > This adds an "&&" everywhere (or in fact, it just skips any existing

> > one, it just has the same effect of adding it everywhere).  I tested it

> > with building gcc and Linux for all supported targets (31 of them; I do

> > some with multiple configs, mostly 32-bit and 64-bit).  None had any

> > difference before and after the change.

> >

> > So I am no longer worried that there will be any fallout from doing

> > this.  There are many things that *could* go wrong, but I don't think

> > there will be enough at all to be an impediment to just throwing the

> > switch.

> >

> > If we go this way no target will need any significant fixing, maybe none

> > at all will be needed across all targets.  And no changes will be needed

> > anywhere immediately.  We could make leading "&&" deprecated, and the

> > same for split condition "1" (which was "&& 1").  This is easy to change

> > automatically as well.

> >

> Thanks very much for doing this!

>

> I guess we are not going to backport this?  If we won't, it seems to need

> some way to ensure the implied "&&" will show up explicitly when backporting

> some define_insn_and_split.


For this reason I'd prefer the explicit "&& ", Seghers testing means
mass-changing all define_insn_and_split is reasonable.

Richard.

> BR,

> Kewen
Segher Boessenkool June 8, 2021, 12:30 p.m. | #26
On Tue, Jun 08, 2021 at 09:08:56AM +0200, Richard Biener wrote:
> On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches

> <gcc-patches@gcc.gnu.org> wrote:

> > on 2021/6/8 上午7:50, Segher Boessenkool wrote:

> > > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:

> > >> 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.

> > >

> > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html

> > >

> > > This adds an "&&" everywhere (or in fact, it just skips any existing

> > > one, it just has the same effect of adding it everywhere).  I tested it

> > > with building gcc and Linux for all supported targets (31 of them; I do

> > > some with multiple configs, mostly 32-bit and 64-bit).  None had any

> > > difference before and after the change.

> > >

> > > So I am no longer worried that there will be any fallout from doing

> > > this.  There are many things that *could* go wrong, but I don't think

> > > there will be enough at all to be an impediment to just throwing the

> > > switch.

> > >

> > > If we go this way no target will need any significant fixing, maybe none

> > > at all will be needed across all targets.  And no changes will be needed

> > > anywhere immediately.  We could make leading "&&" deprecated, and the

> > > same for split condition "1" (which was "&& 1").  This is easy to change

> > > automatically as well.

> > >

> > Thanks very much for doing this!

> >

> > I guess we are not going to backport this?  If we won't, it seems to need

> > some way to ensure the implied "&&" will show up explicitly when backporting

> > some define_insn_and_split.

> 

> For this reason I'd prefer the explicit "&& ", Seghers testing means

> mass-changing all define_insn_and_split is reasonable.


So mass-change all define_insn_and_split where the split condition is
not inclusive of the insn condition (as written, i.e. before the
iterators have added stuff), to be separate define_insn and define_split
patterns?  And *then* add the &&?


Segher
Richard Biener via Gcc-patches June 8, 2021, 12:50 p.m. | #27
On Tue, Jun 8, 2021 at 2:32 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>

> On Tue, Jun 08, 2021 at 09:08:56AM +0200, Richard Biener wrote:

> > On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches

> > <gcc-patches@gcc.gnu.org> wrote:

> > > on 2021/6/8 上午7:50, Segher Boessenkool wrote:

> > > > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:

> > > >> 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.

> > > >

> > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html

> > > >

> > > > This adds an "&&" everywhere (or in fact, it just skips any existing

> > > > one, it just has the same effect of adding it everywhere).  I tested it

> > > > with building gcc and Linux for all supported targets (31 of them; I do

> > > > some with multiple configs, mostly 32-bit and 64-bit).  None had any

> > > > difference before and after the change.

> > > >

> > > > So I am no longer worried that there will be any fallout from doing

> > > > this.  There are many things that *could* go wrong, but I don't think

> > > > there will be enough at all to be an impediment to just throwing the

> > > > switch.

> > > >

> > > > If we go this way no target will need any significant fixing, maybe none

> > > > at all will be needed across all targets.  And no changes will be needed

> > > > anywhere immediately.  We could make leading "&&" deprecated, and the

> > > > same for split condition "1" (which was "&& 1").  This is easy to change

> > > > automatically as well.

> > > >

> > > Thanks very much for doing this!

> > >

> > > I guess we are not going to backport this?  If we won't, it seems to need

> > > some way to ensure the implied "&&" will show up explicitly when backporting

> > > some define_insn_and_split.

> >

> > For this reason I'd prefer the explicit "&& ", Seghers testing means

> > mass-changing all define_insn_and_split is reasonable.

>

> So mass-change all define_insn_and_split where the split condition is

> not inclusive of the insn condition (as written, i.e. before the

> iterators have added stuff), to be separate define_insn and define_split

> patterns?  And *then* add the &&?


Possible.  Maybe first enable a warning for the case with not starting
with "&& "
to give maintainers a chance to fix them and only then do the mass change
to separate define_insn and define_split to avoid churn in the .md files.  Well
maintained ports should be quick to fixup themselves.

And some cases might even be obvious.

Richard.

>

> Segher