Richard Biener <richard.guent...@gmail.com> writes: > On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <li...@linux.ibm.com> wrote: >> >> on 2021/6/2 下午5:13, Richard Sandiford wrote: >> > "Kewen.Lin" <li...@linux.ibm.com> writes: >> >> Hi Richard, >> >> >> >> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote: >> >>> Kewen Lin <li...@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
