Hi Segher, on 2021/3/20 上午5:58, Segher Boessenkool wrote: > On Fri, Mar 19, 2021 at 11:50:41PM +0800, Kewen.Lin wrote: >>> I am curious if the splitters ever triggered where they should not have? >> >> Do you have any suggestion to catch this? I thought the regression >> testing probably can show something different but it didn't unfortunately. > > Well, you can compare the generated binaries with a compiler binary > before and after the patch, for something that will match the splitters, > so with a lot of float<->integer conversions for example. > > I usually look at cc1 or vmlinux, but those both are nice for integer > code only. > > Maybe we should have some tool that for every define_insn finds which > define_splits can possibly match it. >
Thanks for the suggestion. I did the binary comparison for SPEC2017 executables built before and after this patch on both Power8 and Power9, I still didn't see any differences, it seems hard to trigger it. Committed via r11-7756. >>> You might want to make this error easier to detect? Maybe make >>> define_insn_and_split raise an error if the split condition is empty but >>> the insn condition is not? >> >> Good idea! Is there any possible reason to write this kind of >> define_insn_and_split? If no (we should forbid it), I think we can add >> the check and raise an error if hits in gensupport.c. >> >> I'll send out a RFC once GCC12 starts. > > There is no good reason for it. If you really want a > define_insn_and_split that splits more than just that insn, you should > just write the define_insn and the define_split separately. Much > clearer that way. A reader does not expect a define_insn_and_split to > split any other insns. (Or they should not, IMO, of course :-) ) > > And yes, because that might well break things for some targets, or > change behaviour at least, it is a GCC 12 thing. It will be a nice > cleanup though :-) > Got it! Thanks for the further explanation. :) BR, Kewen
