[RFC/PATCH 00/11] Fix up some unexpected empty split conditions

Jeff Law jeffreyalaw@gmail.com
Wed Jun 2 18:25:10 GMT 2021



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


More information about the Gcc-patches mailing list