On Tue, Jun 8, 2021 at 2:27 PM Segher Boessenkool <seg...@kernel.crashing.org> wrote: > > 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.
I wonder if it makes sense to provide ports a means to opt-in into the strict "&& " requirement and thus we can gradually fix them. Probably requires some t-$target make fragment editing plus passing an extra arg to gen* based on that. That way maintainers can gradually fix their ports and make sure they won't regress again. Richard. > > Segher
