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

Richard Sandiford richard.sandiford@arm.com
Thu Jun 3 08:05:02 GMT 2021


"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi Richi/Richard/Jeff/Segher,
>
> Thanks for the comments!
>
> on 2021/6/3 锟斤拷锟斤拷7:52, Segher Boessenkool wrote:
>> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:
>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>> 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.
>> 
>> I'd rather not have that -- we can just write separate define_insn and
>> define_split in that case.
>> 
>
> Not sure if someone would argue that he/she would like to go with one shared
> pattern as before, to avoid any possible differences between two seperated
> patterns and have good maintainability (like only editing on place) and
> slightly better efficiency.

Right.  Plus it creates less make-work.  If we didn't have it, someone
would need to split the define_insn_and_splits that don't currently
use "&&", then later someone might decide that the missing "&&" was a
mistake and need to put them together again (or just revert the patch
and edit from there, I guess).

Plus define_independent_insn_and_split would act as a flag for something
that might be suspect.  If we split them then the define_split condition
will seem to have been chosen deliberately in isolation.

>> How many such cases *are* there?  There are no users exposed to this,
>> and when the split condition is required to start with "&&" (instead of
>> getting that implied) it is not a silent change ever, either.
>> 
>
> If I read the proposal right, the explicit "&&" is only required when going
> to find all potential problematic places for final implied "&&" change.
> But one explicit "&&" does offer good readability.

I don't know.  "&& 1" looks kind of weird to me.

One thing I'd been wondering about a while ago was whether we should key
the split part of define_insn_and_splits off the insn code, instead of
repeating the pattern match and insn C condition.  That would make the
split apply only to the associated define_insns, whereas at the moment
they also apply to any earlier (less general) define_insn that happens
to match the pattern and the C conditions.  It would also reduce the
complexity of the autogenerated define_split logic.

I don't know whether that's a good idea or not.  But having an explicit
"&&" implies that the generator shouldn't do that, and that it should
retest the insn condition from scratch.

>>> - 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 "&&".
>> 
>> If we make that change permanently, that is all steps we ever need!
>> 
>
> So the question is that: whether we need to demand an explicit "&&".
> Richard's proposal is for answer "no" which aligns with Richi's auto
> filling advice before.  I think it would result in fewer changes since
> those places without explicit "&&" are mostly unintentional, all the jobs
> are done by implied "&&".  Its downside seems to be bad readability, new
> readers may take it as two seperated conditions at first glance, but I
> guess if we emphasize this change in the document it would be fine?
> Or emitting one warning if missing an explicit "&&"?

IMO the natural way to read it is that the split C condition gives the
conditions under which the instruction should be split.  I think that's
why forgetting the "&&" is such a common mistake.  (I know I've done it
plenty of times.)

IMO requiring the "&&" is baking in an alternative, less intuitive,
interpretation.

Thanks,
Richard


More information about the Gcc-patches mailing list