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 :-) > 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) > + 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. > + else if (lp_nr == 0 || lp_nr == INT_MIN) > + { > + /* nothrow notes can be dropped but we preserve > + those on calls. */ > + if (from_insn == i3 && CALL_P (i3)) > + place = i3; > + } > + else > + { > + /* Distribute must-not-throw notes to all resulting > + insns and just pick the first. */ > + if (!find_reg_note (i3, REG_EH_REGION, NULL_RTX) > + && (CALL_P (i3) > + || (cfun->can_throw_non_call_exceptions > + && may_trap_p (i3)))) > + place = i3; > + if (i2 > + && !find_reg_note (i2, REG_EH_REGION, NULL_RTX) > + && cfun->can_throw_non_call_exceptions > + && may_trap_p (i2)) > + { > + if (place) > + place2 = i2; > + else > + place = i2; > + } > + } > + break; > + } Okay for trunk with the spello fixed, and maybe one more assert added. Thanks for all the work on this! Segher