On Thu, 21 Apr 2022, Richard Biener wrote:
> On Wed, 20 Apr 2022, Segher Boessenkool wrote:
>
> > Hi!
> >
> > This looks great :-)
> >
> > On Wed, Apr 20, 2022 at 03:52:33PM +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 note with lp > 0 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.
> > >
> > > For REG_EH_REGION corresponding to must-not-throw regions or
> > > nothrow marking try_combine gets new code ensuring we can merge
> > > and distribute notes which means placing must-not-throw notes
> > > on all result insns, and dropping nothrow notes or preserve
> > > them on i3 for calls.
> >
> > > * combine.cc (distribute_notes): Assert that a REG_EH_REGION
> > > with landing pad > 0 is from i3 and only keep it there or drop
> > > it if the insn can not trap. Throw away REG_EH_REGION with
> > > landing pad = 0 or INT_MIN if it does not originate from a
> > > call i3. Distribute must-not-throw REG_EH_REGION to all
> > > resulting instructions.
> > > (try_combine): Ensure that we can merge REG_EH_REGION notes.
> >
> > > --- a/gcc/combine.cc
> > > +++ b/gcc/combine.cc
> > > @@ -2951,6 +2951,45 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn
> > > *i1, rtx_insn *i0,
> > > return 0;
> > > }
> > >
> > > + /* When i3 transfers to an EH handler we cannot combine if any of the
> > > + sources are within a must-not-throw region. Else we can throw away
> > > + any nothrow, pick a random must-not-throw region or preserve the EH
> > > + transfer on i3. Since we want to preserve nothrow notes on calls
> > > + we have to avoid combining from must-not-throw stmts there as well.
> > > + This has to be kept in sync with distribute_note. */
> > > + if (rtx i3_eh = find_reg_note (i3, REG_EH_REGION, NULL_RTX))
> > > + {
> > > + int i3_lp_nr = INTVAL (XEXP (i3_eh, 0));
> > > + if (i3_lp_nr > 0
> > > + || ((i3_lp_nr == 0 || i3_lp_nr == INT_MIN) && CALL_P (i3)))
> > > + {
> > > + rtx eh;
> > > + int eh_lp;
> > > + if (((eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> > > + && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > + && eh_lp != INT_MIN)
> > > + || (i2
> > > + && (eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> > > + && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > + && eh_lp != INT_MIN)
> > > + || (i1
> > > + && (eh = find_reg_note (i1, REG_EH_REGION, NULL_RTX))
> > > + && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > + && eh_lp != INT_MIN)
> > > + || (i0
> > > + && (eh = find_reg_note (i0, REG_EH_REGION, NULL_RTX))
> > > + && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > + && eh_lp != INT_MIN))
> > > + {
> > > + if (dump_file && (dump_flags & TDF_DETAILS))
> > > + fprintf (dump_file, "Can't combine insn in must-not-throw "
> > > + "EH region into i3 which can throws\n");
> > > + undo_all ();
> > > + return 0;
> > > + }
> > > + }
> > > + }
> >
> > The assignments in the conditionals make this hard to read, and harder
> > to change, btw. A utility function wouldn't hurt? The problem of
> > course would be thinking of a good name for it :-)
>
> I've added insn_must_not_throw_p to except.{cc,h}.
>
> > > case REG_EH_REGION:
> > > - /* These notes must remain with the call or trapping instruction. */
> > > - if (CALL_P (i3))
> > > - place = i3;
> > > - else if (i2 && CALL_P (i2))
> > > - place = i2;
> > > - else
> > > - {
> > > - gcc_assert (cfun->can_throw_non_call_exceptions);
> > > - if (may_trap_p (i3))
> > > - place = i3;
> > > - else if (i2 && may_trap_p (i2))
> > > - place = i2;
> > > - /* ??? Otherwise assume we've combined things such that we
> > > - can now prove that the instructions can't trap. Drop the
> > > - note in this case. */
> > > - }
> > > - break;
> > > + {
> > > + /* This handling needs to be kept in sync with the
> > > + prerequesite checking in try_combine. */
> >
> > (prerequisite)
>
> Fixed.
>
> > > + int lp_nr = INTVAL (XEXP (note, 0));
> > > + /* A REG_EH_REGION note transfering control can only ever come
> > > + from i3 and it has to stay there. */
> > > + if (lp_nr > 0)
> > > + {
> > > + gcc_assert (from_insn == i3);
> > > + if (CALL_P (i3))
> > > + place = i3;
> > > + else
> > > + {
> > > + gcc_assert (cfun->can_throw_non_call_exceptions);
> > > + /* If i3 can still trap preserve the note, otherwise we've
> > > + combined things such that we can now prove that the
> > > + instructions can't trap. Drop the note in this case. */
> > > + if (may_trap_p (i3))
> > > + place = i3;
> > > + }
> > > + }
> >
> > It isn't immediately clear to me that we cannot have moved a trapping
> > thing to i2? Do we guarantee that somewhere? Can we / should we assert
> > that here? gcc_assert (!(i2 && may_trap_p (i2))) or something like
> > that.
>
> IMHO asserts for catching things we don't handle are bad. I suppose
> I could add sth like
>
> @@ -3724,7 +3712,12 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn
> *i1, rt
> x_insn *i0,
> || !modified_between_p (*split, i2, i3))
> /* We can't overwrite I2DEST if its value is still used by
> NEWPAT. */
> - && ! reg_referenced_p (i2dest, newpat))
> + && ! reg_referenced_p (i2dest, newpat)
> + /* We should not split a possibly trapping part of a throwing
> + insn. */
> + && (!cfun->can_throw_non_call_exceptions
> + || !can_throw_internal (i3)
> + || !may_trap_p (*split)))
> {
> rtx newdest = i2dest;
> enum rtx_code split_code = GET_CODE (*split);
>
> but then without a testcase it's hard to see if this is to the point.
> It of course also assumes we're never going to split the call part
> from i3 to i2.
>
> It should be at least safe and with that we could add your assert,
> sligthly altered to
>
> gcc_assert (!(cfun->can_throw_non_call_exceptions
> && i2 && may_trap_p (i2)));
>
> because we have to guard against spltting say a trapping memory
> load out from a CALL_P, but only with non-call EH (the call
> might not throw but a (pre-existing!) memory load could trap).
So that doesn't work, it ICEs
FAIL: c52103k
FAIL: c52103p
FAIL: c52104p
FAIL: c95089a
FAIL: cxa4034
that means the
/* If we can split it and use I2DEST, go ahead and see if that
helps things be recognized. Verify that none of the registers
are set between I2 and I3. */
if (insn_code_number < 0
&& (split = find_split_point (&newpat, i3, false)) != 0
...
case isn't the only place we can split i2 (as seen in
distribute_notes) from i3?
This is for example
Trying 901, 902 -> 903:
901: r556:DI=0x9
902: {r555:DI=r556:DI-r132:DI;clobber flags:CC;}
REG_DEAD r556:DI
REG_DEAD r132:DI
REG_UNUSED flags:CC
REG_EQUAL 0x9-r132:DI
903: r256:QI=[r234:DI+r555:DI]
REG_DEAD r555:DI
REG_DEAD r234:DI
REG_EH_REGION 0x9
Successfully matched this instruction:
(set (reg:DI 555)
(minus:DI (reg/f:DI 234 [ arrx52.69 ])
(reg:DI 132 [ _58 ])))
Successfully matched this instruction:
(set (reg:QI 256 [ _320 ])
(mem/j:QI (plus:DI (reg:DI 555)
(const_int 9 [0x9])) [11 (*arrx52.69_259)[9]{lb: _58 sz: 1}+0
S1 A8]))
where it's probably overzealeous, and just because we have at
this point
(insn 902 901 903 134 (parallel [
(set (reg:DI 555)
(minus:DI (reg/f:DI 234 [ arrx52.69 ])
(reg:DI 132 [ _58 ])))
(clobber (reg:CC 17 flags))
]) "c52104p.adb":273:44 303 {*subdi_1}
(expr_list:REG_DEAD (reg/f:DI 234 [ arrx52.69 ])
(nil)))
and may_trap_p does
case EXPR_LIST:
/* An EXPR_LIST is used to represent a function call. This
certainly may trap. */
return 1;
so in this case for -fnon-call-exceptions we are going to reject
a lot of splits - but we actually don't (because the may_trap_p
check on the split point fails). I suppose for the assert
we'd rather want to check may_trap_p (PATTERN (i2)) then?
Do we want to do this for the other (non-call?) cases as well?
Changing the assert to
gcc_assert (!(cfun->can_throw_non_call_exceptions
&& i2 && may_trap_p (PATTERN (i2))));
fixes the ICE (just checked one of the testcases).
I'm going to re-check with this, but given may_trap_p is looking
to err on the trapping side too easily maybe an assert isn't a good
idea.
Richard.
