On Tue, 19 Apr 2022, Segher Boessenkool wrote:
> Hi!
>
> So the assert last week was for a landing pad <= 0? < or =?
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.
> On Tue, Apr 19, 2022 at 01:02:09PM +0200, Richard Biener wrote:
> > The following mitigates a problem in combine distribute_notes which
> > places an original REG_EH_REGION based on only may_trap_p which is
> > good to test whether a non-call insn can possibly throw but not if
> > actually it does or we care. That's something we decided at RTL
> > expansion time where we possibly still know the insn evaluates
> > to a constant.
> >
> > In fact, the REG_EH_REGION can only come from the original i3 and
> > an assert is added to that effect. That means we only need to
> > retain the note on i3 or, if that cannot trap, drop it but we
> > should never move it to i2. If splitting of i3 ever becomes a
> > problem here the insn split should be rejected instead.
> >
> > 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.
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). 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).
> > + {
> > + int lp_nr = INTVAL (XEXP (note, 0));
> > + /* 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.
> > + /* 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?)
> Okay for trunk with maybe a word or two more there. Thanks!
I'll see if there are more comments before pushing.
Thanks,
Richard.
