| Message ID | cover.1622179420.git.linkw@linux.ibm.com |
|---|---|
| Headers | show |
| Series | Fix up some unexpected empty split conditions | expand |
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(-)
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
"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
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
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 <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
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
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
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 >
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
"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
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));
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
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
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
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
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
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
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];
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
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)); > >
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
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)); >>>
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
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
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
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