| Message ID | 20220419110209.A4C1F139BE@imap2.suse-dmz.suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] rtl-optimization/105231 - distribute_notes and REG_EH_REGION | expand |
Hi! So the assert last week was for a landing pad <= 0? < or =? 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? > + { > + 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 ;-) > + /* 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?) Okay for trunk with maybe a word or two more there. Thanks! Segher
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.
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? > 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. > 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? :-) > > > + /* 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. > > > + /* 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. combine does not consider EH_REGIONs anywhere else. It uses insn_nothrow_p in some places though. Segher
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
On Tue, Apr 19, 2022 at 05:00:12PM +0200, Richard Biener wrote: > On Tue, 19 Apr 2022, Segher Boessenkool wrote: > > > > 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. Losing optimisation is always safe. If removing a must-not-throw is not safe we should assert this does not happen (or if we know it does happen, actually fix that). > 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 ;) The notes originally from i3 are distributed before those from i2, which are before those from i1, and those are distributed before those from i0. > 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. Up until /* Get the old REG_NOTES and LOG_LINKS from all our insns and clear them. */ all old notes are still intact. The insns are i3, i2, etc.; their patterns can change during combine, but the insns themselves don't. > > > 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. That is impossible to get right, in general. That is why there is a from_insn argument to distribute_notes. If you first move everything to one combined insn you would have to pry things apart again :-( > > > 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. Yes. > > None of the insns other than i3 are call insns, ever. > > Good. An understatement :-) Segher
diff --git a/gcc/combine.cc b/gcc/combine.cc index 53dcac92abc..ca9d6f0e6e0 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -14175,23 +14175,33 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, break; 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; + { + 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); + /* 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. */ + if (from_insn != i3) + ; + /* Otherwise the note must remain with the call or trapping + instruction. */ + else 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; + } + break; + } case REG_ARGS_SIZE: /* ??? How to distribute between i3-i1. Assume i3 contains the diff --git a/gcc/testsuite/gcc.dg/torture/pr105231.c b/gcc/testsuite/gcc.dg/torture/pr105231.c new file mode 100644 index 00000000000..50459219c08 --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr105231.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target int32plus } */ +/* { dg-require-effective-target dfp } */ +/* { dg-additional-options "-fsanitize-coverage=trace-pc -fnon-call-exceptions --param=max-cse-insns=1 -frounding-math" } */ +/* { dg-additional-options "-mstack-arg-probe" { target x86_64-*-* i?86-*-* } } */ + +void baz (int *); +void bar (double, double, _Decimal64); + +void +foo (void) +{ + int s __attribute__((cleanup (baz))); + bar (0xfffffffffffffffe, 0xebf3fff2fbebaf7f, 0xffffffffffffff); +}