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
