On Tue, 19 Apr 2022, Segher Boessenkool wrote: > On Tue, Apr 19, 2022 at 02:58:26PM +0200, Richard Biener wrote: > > On Tue, 19 Apr 2022, Segher Boessenkool wrote: > > The assert was for any landing pad which obviously failed - the > > testsuite fails were all for MUST_NOT_THROW (negative) regions > > which do not end basic-blocks. > > I see, thanks. > > > > > We are also considering all REG_EH_REGION equal, including > > > > must-not-throw and nothrow kinds but when those are not from i3 > > > > we have no good idea what should happen to them, so the following > > > > simply drops them. > > > > > > And that always is safe? Why do we have REG_EH_REGION for those cases > > > at all, then? > > > > It's the only "safe" thing to do at distribute_notes time I think. If > > it is not "safe" (it might lose must-not-throw EH events, or lose > > optimization when dropping nothrow) we probably have to reject the > > combination earlier. > > So assert this does not happen please?
I'm not sure how I can. AFAICS I cannot rely on all other REG_EH_REGION notes be already present on 'i3' when processing notes from the original i1 or i2. I can only assert we never have any REG_EH_REGION notes from i1 or i2 but I already know we do from the last round of testsuite failures ;) If you can point me to a (hopefully single) place in combine.cc where the set of original source insns is fixed and the original notes still present I can come up with a test for what should be safe input. > > As I understand combining to multiple insns always happens via > > a split (so we combine up to three insns into one and then might > > end up splitting that into at most two insns). > > Yes, but not necessarily a define_split or similar: combine itself knows > how to split things, too. The obvious one is a parallel of multiple > sets, but it can also break the rhs of a set apart, for example. I see. So I was aiming at "distributing" the notes to the single combined insn _before_ splitting it and then making the split process DTRT - that's much easier to get right than the current setup where we receive notes from random insns a time to be distributed to another random insn. > > The only case we > > could in theory special-case is when _all_ original insns combined > > have the exact same REG_EH_REGION (all have it with the same > > landing pad number) or some have none but i3 at least has one. > > Then we should be able to distribute the note to all possibly > > two result insns. But I can't see that distribute_note has > > this info readily available (that there not exist conflicting > > REG_EH_REGIONs, like MNT on the original i2 and a > 0 one on i3). > > Not sure this would be worth the complexity. Do we see this happen > ever, can we even test it? :-) We cannot test this in distribute_notes, we could test for this when we have all source insns and reject the cases we cannot possibly recover from in the current distribute_notes implementation. > > > > + /* A REG_EH_REGION note transfering control can only ever > > > > come > > > > + from i3. */ > > > > + gcc_assert (lp_nr <= 0 || from_insn == i3); > > > > > > if (lp_nr > 0) > > > gcc_assert (from_insn == i3); > > > is less obfuscated ;-) > > > > I find that less obvious, but you can have it your way if you like. > > It corresponds more directly to the comment, the narrative guides the > reader? But please use whichever you think best. I've adjusted to how you like it. > > > > + /* For nothrow (lp_nr == 0 or lp_nr == INT_MIN) and > > > > + for insns in a MUST_NOT_THROW region (lp_nr < 0) > > > > + it's difficult to decide what to do for notes > > > > + coming from an insn that is not i3. Just drop > > > > + those. */ > > > > > > That sounds like we do not know what is correct to do, so just sweep it > > > under the carpet and hope it works out. "Just drop those, that is > > > always safe"? (Is it always safe?) > > > > If it is not safe we should have rejected the combination. I fully > > expect that we'd need to have a piece during analysis constraining > > what cases we feed into here to be really "safe". I'm really not > > familiar with combine so I know nothing of the constraints it has > > (like is only i3 ever going to be a CALL_INSN_P?) > > https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/combine.cc;h=53dcac92abc248a80fc32dd1d3bb641a650d4d9a;hb=HEAD#l1882 > https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/combine.cc;h=53dcac92abc248a80fc32dd1d3bb641a650d4d9a;hb=HEAD#l2644 > > None of the insns other than i3 are call insns, ever. Good. Richard. > combine does not consider EH_REGIONs anywhere else. It uses > insn_nothrow_p in some places though. > > Segher
