On 6/2/2021 11:32 AM, Richard Sandiford wrote:
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.
That works for me. If we'd had this in place earlier I wouldn't have
mucked up the H8 port.
jeff
