On Tue, Jun 08, 2021 at 09:05:57AM +0200, Richard Biener wrote:
> On Tue, Jun 8, 2021 at 12:05 AM Segher Boessenkool
> <seg...@kernel.crashing.org> wrote:
> >
> > In theory we could have a split condition not inclusive of the insn
> > condition in the past. That never was a good idea, the code does not do
> > what a non-suspicious reader would think it does. But it leads to more
> > serious problems together with iterators: if the split condition (as
> > written) does not start with "&&", you do not get the insn condition
> > included in the split condition, and that holds for the part of the insn
> > condition that was generated by the iterator as well!
> >
> > This patch simply always joins the two conditions (after the iterators
> > have done their work) to get the effective split condition.
> >
> > I tested this on all Linux targets, building the Linux kernel for each,
> > and it does not change generated code for any of them, so I think we do
> > not have much breakage to fear. But it is possible for other targets of
> > course, and for floating point or vector code, etc.
> >
> > Is this okay for trunk?
>
> Even if it looks uglier I would prefer to enforce a leading "&& " on the
> split condition. That keeps the semantic of the define_insn_and_split
> the same on trunk and branches and thus maintaining things easier.
> I suppose once branches without such enforcement go out of
> maintainance we can mass-strip the "&& "s.
This still allows a leading &&, but it doesn't enforce it. Since we
have survived for years and years without enforcing this I don't foresee
any big problems. There should not be many backports able to trigger
this either.
> I guess a mass-change to add "&& "s at this point is smaller than
> a corresponding change to drop them (IMHO leaving both after this
> change would be confusing).
I also managed to build with nds32 now (it is one of those targets that
likes to ICE with a Linux defconfig), and it *does* show differences.
Looking at the machine description there are many patterns that have
!TARGET_BIG_ENDIAN in the insn condition but not in the split condition.
That is exactly the kind of situation that is almost certainly an error
(or "not by design", or "very bad design", take your pick). The config
I build is LE so none of these insns match, but apparently the split
condition *does* trigger for some insns matched by *other* patterns.
There also is "sms1", which has insn condition
"NDS32_EXT_DSP_P ()
&& (!reload_completed
|| !nds32_need_split_sms_p (operands[3], operands[4],
operands[5], operands[6]))"
but split condition
"NDS32_EXT_DSP_P ()
&& !reload_completed
&& nds32_need_split_sms_p (operands[3], operands[4],
operands[5], operands[6])"
Luckily we never trigger that.
So yeah, patch withdrawn. This on one hand is proof we do want to make
such a change, but on the other hand shows it needs more preparatory
steps.
Segher
