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). BR, Kewen
