| Message ID | a98c01571d8f09ab1faecf5790d61d1273f8746b.1623093126.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
| Series | rtl: Join the insn and split conditions in define_insn_and_split | expand |
On Tue, Jun 8, 2021 at 12:05 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > In theory we could have a split condition not inclusive of the insn > condition in the past. That never was a good idea, the code does not do > what a non-suspicious reader would think it does. But it leads to more > serious problems together with iterators: if the split condition (as > written) does not start with "&&", you do not get the insn condition > included in the split condition, and that holds for the part of the insn > condition that was generated by the iterator as well! > > This patch simply always joins the two conditions (after the iterators > have done their work) to get the effective split condition. > > I tested this on all Linux targets, building the Linux kernel for each, > and it does not change generated code for any of them, so I think we do > not have much breakage to fear. But it is possible for other targets of > course, and for floating point or vector code, etc. > > Is this okay for trunk? Even if it looks uglier I would prefer to enforce a leading "&& " on the split condition. That keeps the semantic of the define_insn_and_split the same on trunk and branches and thus maintaining things easier. I suppose once branches without such enforcement go out of maintainance we can mass-strip the "&& "s. I guess a mass-change to add "&& "s at this point is smaller than a corresponding change to drop them (IMHO leaving both after this change would be confusing). Richard. > > Segher > > > 2021-06-07 Segher Boessenkool <segher@kernel.crashing.org> > > * gensupport.c (process_rtx) [DEFINE_INSN_AND_SPLIT]: Always include > the insn condition in the split condition. > > --- > gcc/gensupport.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/gcc/gensupport.c b/gcc/gensupport.c > index 2cb760ffb90f..8a6345d36470 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; > @@ -609,17 +608,25 @@ process_rtx (rtx desc, file_location loc) > remove_constraints (XVECEXP (split, 0, i)); > } > > - /* 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) > + && GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > + error_at (loc, "the rewrite condition must start with `&&'"); > + > + /* If the split condition starts with "&&", skip that. */ > + 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 + 2); > + split_cond += 2; > } > - else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > - error_at (loc, "the rewrite condition must start with `&&'"); > + > + /* Always use the conjunction of the given split condition and the > + insn condition (which includes stuff from iterators, it is not just > + what is given in the pattern in the machine description) as the > + split condition to use. */ > + split_cond = rtx_reader_ptr->join_c_conditions (insn_cond, split_cond); > + > XSTR (split, 1) = split_cond; > if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) > XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1)); > -- > 1.8.3.1 >
On Tue, Jun 08, 2021 at 09:05:57AM +0200, Richard Biener wrote: > On Tue, Jun 8, 2021 at 12:05 AM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > > > In theory we could have a split condition not inclusive of the insn > > condition in the past. That never was a good idea, the code does not do > > what a non-suspicious reader would think it does. But it leads to more > > serious problems together with iterators: if the split condition (as > > written) does not start with "&&", you do not get the insn condition > > included in the split condition, and that holds for the part of the insn > > condition that was generated by the iterator as well! > > > > This patch simply always joins the two conditions (after the iterators > > have done their work) to get the effective split condition. > > > > I tested this on all Linux targets, building the Linux kernel for each, > > and it does not change generated code for any of them, so I think we do > > not have much breakage to fear. But it is possible for other targets of > > course, and for floating point or vector code, etc. > > > > Is this okay for trunk? > > Even if it looks uglier I would prefer to enforce a leading "&& " on the > split condition. That keeps the semantic of the define_insn_and_split > the same on trunk and branches and thus maintaining things easier. > I suppose once branches without such enforcement go out of > maintainance we can mass-strip the "&& "s. This still allows a leading &&, but it doesn't enforce it. Since we have survived for years and years without enforcing this I don't foresee any big problems. There should not be many backports able to trigger this either. > I guess a mass-change to add "&& "s at this point is smaller than > a corresponding change to drop them (IMHO leaving both after this > change would be confusing). I also managed to build with nds32 now (it is one of those targets that likes to ICE with a Linux defconfig), and it *does* show differences. Looking at the machine description there are many patterns that have !TARGET_BIG_ENDIAN in the insn condition but not in the split condition. That is exactly the kind of situation that is almost certainly an error (or "not by design", or "very bad design", take your pick). The config I build is LE so none of these insns match, but apparently the split condition *does* trigger for some insns matched by *other* patterns. There also is "sms1", which has insn condition "NDS32_EXT_DSP_P () && (!reload_completed || !nds32_need_split_sms_p (operands[3], operands[4], operands[5], operands[6]))" but split condition "NDS32_EXT_DSP_P () && !reload_completed && nds32_need_split_sms_p (operands[3], operands[4], operands[5], operands[6])" Luckily we never trigger that. So yeah, patch withdrawn. This on one hand is proof we do want to make such a change, but on the other hand shows it needs more preparatory steps. Segher
On Tue, Jun 8, 2021 at 2:27 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Tue, Jun 08, 2021 at 09:05:57AM +0200, Richard Biener wrote: > > On Tue, Jun 8, 2021 at 12:05 AM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > > > > In theory we could have a split condition not inclusive of the insn > > > condition in the past. That never was a good idea, the code does not do > > > what a non-suspicious reader would think it does. But it leads to more > > > serious problems together with iterators: if the split condition (as > > > written) does not start with "&&", you do not get the insn condition > > > included in the split condition, and that holds for the part of the insn > > > condition that was generated by the iterator as well! > > > > > > This patch simply always joins the two conditions (after the iterators > > > have done their work) to get the effective split condition. > > > > > > I tested this on all Linux targets, building the Linux kernel for each, > > > and it does not change generated code for any of them, so I think we do > > > not have much breakage to fear. But it is possible for other targets of > > > course, and for floating point or vector code, etc. > > > > > > Is this okay for trunk? > > > > Even if it looks uglier I would prefer to enforce a leading "&& " on the > > split condition. That keeps the semantic of the define_insn_and_split > > the same on trunk and branches and thus maintaining things easier. > > I suppose once branches without such enforcement go out of > > maintainance we can mass-strip the "&& "s. > > This still allows a leading &&, but it doesn't enforce it. Since we > have survived for years and years without enforcing this I don't foresee > any big problems. There should not be many backports able to trigger > this either. > > > I guess a mass-change to add "&& "s at this point is smaller than > > a corresponding change to drop them (IMHO leaving both after this > > change would be confusing). > > I also managed to build with nds32 now (it is one of those targets that > likes to ICE with a Linux defconfig), and it *does* show differences. > > Looking at the machine description there are many patterns that have > !TARGET_BIG_ENDIAN in the insn condition but not in the split condition. > That is exactly the kind of situation that is almost certainly an error > (or "not by design", or "very bad design", take your pick). The config > I build is LE so none of these insns match, but apparently the split > condition *does* trigger for some insns matched by *other* patterns. > > There also is "sms1", which has insn condition > "NDS32_EXT_DSP_P () > && (!reload_completed > || !nds32_need_split_sms_p (operands[3], operands[4], > operands[5], operands[6]))" > but split condition > "NDS32_EXT_DSP_P () > && !reload_completed > && nds32_need_split_sms_p (operands[3], operands[4], > operands[5], operands[6])" > Luckily we never trigger that. > > So yeah, patch withdrawn. This on one hand is proof we do want to make > such a change, but on the other hand shows it needs more preparatory > steps. I wonder if it makes sense to provide ports a means to opt-in into the strict "&& " requirement and thus we can gradually fix them. Probably requires some t-$target make fragment editing plus passing an extra arg to gen* based on that. That way maintainers can gradually fix their ports and make sure they won't regress again. Richard. > > Segher
On Tue, Jun 08, 2021 at 02:48:11PM +0200, Richard Biener wrote: > > So yeah, patch withdrawn. This on one hand is proof we do want to make > > such a change, but on the other hand shows it needs more preparatory > > steps. > > I wonder if it makes sense to provide ports a means to opt-in into > the strict "&& " requirement and thus we can gradually fix them. > Probably requires some t-$target make fragment editing plus > passing an extra arg to gen* based on that. > > That way maintainers can gradually fix their ports and make sure > they won't regress again. Just some target macro might be better / easier? Just gensupport.c will need to use it. Will we still allow empty split conditions? And automatically make that do the equivalent of "&& 1"? Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Tue, Jun 08, 2021 at 02:48:11PM +0200, Richard Biener wrote: >> > So yeah, patch withdrawn. This on one hand is proof we do want to make >> > such a change, but on the other hand shows it needs more preparatory >> > steps. >> >> I wonder if it makes sense to provide ports a means to opt-in into >> the strict "&& " requirement and thus we can gradually fix them. >> Probably requires some t-$target make fragment editing plus >> passing an extra arg to gen* based on that. >> >> That way maintainers can gradually fix their ports and make sure >> they won't regress again. > > Just some target macro might be better / easier? Just gensupport.c will > need to use it. > > Will we still allow empty split conditions? And automatically make that > do the equivalent of "&& 1"? Wouldn't that run the risk of the partial transition that my suggestion was rejected for? ;-) I think an empty define_insn_and_split split condition should continue to mean the same thing everywhere. So while we continue to have ports in which an empty condition means one thing, I don't think we should also have ports where an empty condition means something else. Thanks, Richard
On Tue, Jun 08, 2021 at 04:50:56PM +0100, Richard Sandiford wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Tue, Jun 08, 2021 at 02:48:11PM +0200, Richard Biener wrote: > >> > So yeah, patch withdrawn. This on one hand is proof we do want to make > >> > such a change, but on the other hand shows it needs more preparatory > >> > steps. > >> > >> I wonder if it makes sense to provide ports a means to opt-in into > >> the strict "&& " requirement and thus we can gradually fix them. > >> Probably requires some t-$target make fragment editing plus > >> passing an extra arg to gen* based on that. > >> > >> That way maintainers can gradually fix their ports and make sure > >> they won't regress again. > > > > Just some target macro might be better / easier? Just gensupport.c will > > need to use it. > > > > Will we still allow empty split conditions? And automatically make that > > do the equivalent of "&& 1"? > > Wouldn't that run the risk of the partial transition that my suggestion > was rejected for? ;-) First off, I have changed position a few times now on what I think would be the best way forward here :-) I assumed we would make the "&&" requirement a requirement for GCC 12 eventually. But yes that needs to be spelled out! > I think an empty define_insn_and_split split condition should continue > to mean the same thing everywhere. So while we continue to have ports > in which an empty condition means one thing, I don't think we should > also have ports where an empty condition means something else. If we have the "&&" requirement, we either disallow empty split conditions, or have it be treated as "&& 1". In either case it will mean the same thing everywhere. And in all cases, not just these cases but *all* cases, code that works on trunk will not necessarily work on backports. I don't see any obvious cases where this will be a worse problem with this, do you? Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Tue, Jun 08, 2021 at 04:50:56PM +0100, Richard Sandiford wrote: >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> > On Tue, Jun 08, 2021 at 02:48:11PM +0200, Richard Biener wrote: >> >> > So yeah, patch withdrawn. This on one hand is proof we do want to make >> >> > such a change, but on the other hand shows it needs more preparatory >> >> > steps. >> >> >> >> I wonder if it makes sense to provide ports a means to opt-in into >> >> the strict "&& " requirement and thus we can gradually fix them. >> >> Probably requires some t-$target make fragment editing plus >> >> passing an extra arg to gen* based on that. >> >> >> >> That way maintainers can gradually fix their ports and make sure >> >> they won't regress again. >> > >> > Just some target macro might be better / easier? Just gensupport.c will >> > need to use it. >> > >> > Will we still allow empty split conditions? And automatically make that >> > do the equivalent of "&& 1"? >> >> Wouldn't that run the risk of the partial transition that my suggestion >> was rejected for? ;-) > > First off, I have changed position a few times now on what I think would > be the best way forward here :-) > > I assumed we would make the "&&" requirement a requirement for GCC 12 > eventually. But yes that needs to be spelled out! > >> I think an empty define_insn_and_split split condition should continue >> to mean the same thing everywhere. So while we continue to have ports >> in which an empty condition means one thing, I don't think we should >> also have ports where an empty condition means something else. > > If we have the "&&" requirement, we either disallow empty split > conditions, or have it be treated as "&& 1". In either case it will > mean the same thing everywhere. Ah, OK. I meant that we shouldn't change what an empty condition means until the transition is complete and the target macro has been removed. If the question was instead whether we should allow an empty condition once the transition is complete, then I've no opinion either way. (It doesn't seem like something we need to decide now though.) Thanks, Richard
diff --git a/gcc/gensupport.c b/gcc/gensupport.c index 2cb760ffb90f..8a6345d36470 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; @@ -609,17 +608,25 @@ process_rtx (rtx desc, file_location loc) remove_constraints (XVECEXP (split, 0, i)); } - /* 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) + && GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) + error_at (loc, "the rewrite condition must start with `&&'"); + + /* If the split condition starts with "&&", skip that. */ + 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 + 2); + split_cond += 2; } - else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) - error_at (loc, "the rewrite condition must start with `&&'"); + + /* Always use the conjunction of the given split condition and the + insn condition (which includes stuff from iterators, it is not just + what is given in the pattern in the machine description) as the + split condition to use. */ + split_cond = rtx_reader_ptr->join_c_conditions (insn_cond, split_cond); + XSTR (split, 1) = split_cond; if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE) XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));