* [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927) @ 2021-04-18 15:03 Segher Boessenkool 2021-04-18 15:24 ` Jakub Jelinek 0 siblings, 1 reply; 4+ messages in thread From: Segher Boessenkool @ 2021-04-18 15:03 UTC (permalink / raw) To: gcc-patches; +Cc: jakub, Segher Boessenkool If the register named in an existing REG_UNUSED note dies somewhere between where the note used to be and I3, we should just drop it. 2021-04-21 Segher Boessenkool <segher@kernel.crashing.org> PR rtl-optimization/99927 * combine.c (distribute_notes) [REG_UNUSED]: If the register already is dead, just drop it. --- Committed to trunk. This will need backports to all open branches. Segher gcc/combine.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gcc/combine.c b/gcc/combine.c index 9063a07bd009..62bf4aeaabae 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -14366,6 +14366,11 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, we keep notes from i2 or i1 if they will turn into REG_DEAD notes. */ + /* If this register is set or clobbered between FROM_INSN and I3, + we should not create a note for it. */ + if (reg_set_between_p (XEXP (note, 0), from_insn, i3)) + break; + /* If this register is set or clobbered in I3, put the note there unless there is one already. */ if (reg_set_p (XEXP (note, 0), PATTERN (i3))) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927) 2021-04-18 15:03 [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927) Segher Boessenkool @ 2021-04-18 15:24 ` Jakub Jelinek 2021-04-18 17:03 ` Segher Boessenkool 0 siblings, 1 reply; 4+ messages in thread From: Jakub Jelinek @ 2021-04-18 15:24 UTC (permalink / raw) To: Segher Boessenkool; +Cc: gcc-patches On Sun, Apr 18, 2021 at 03:03:07PM +0000, Segher Boessenkool wrote: > If the register named in an existing REG_UNUSED note dies somewhere > between where the note used to be and I3, we should just drop it. > > 2021-04-21 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/99927 > * combine.c (distribute_notes) [REG_UNUSED]: If the register already > is dead, just drop it. > --- > > Committed to trunk. This will need backports to all open branches. Thanks for working on this. Just some nits but note that I don't know much about the combiner... > gcc/combine.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/gcc/combine.c b/gcc/combine.c > index 9063a07bd009..62bf4aeaabae 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -14366,6 +14366,11 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, > we keep notes from i2 or i1 if they will turn into REG_DEAD > notes. */ > > + /* If this register is set or clobbered between FROM_INSN and I3, > + we should not create a note for it. */ > + if (reg_set_between_p (XEXP (note, 0), from_insn, i3)) > + break; > + > /* If this register is set or clobbered in I3, put the note there > unless there is one already. */ > if (reg_set_p (XEXP (note, 0), PATTERN (i3))) Doesn't this make the if (from_insn != i3 && i2 && INSN_P (i2) && reg_referenced_p (XEXP (note, 0), PATTERN (i2))) { if (!reg_set_p (XEXP (note, 0), PATTERN (i2))) PUT_REG_NOTE_KIND (note, REG_DEAD); if (! (REG_P (XEXP (note, 0)) ? find_regno_note (i2, REG_NOTE_KIND (note), REGNO (XEXP (note, 0))) : find_reg_note (i2, REG_NOTE_KIND (note), XEXP (note, 0)))) place = i2; } case unreachable (the reg_set_p stuff at least; I mean if reg_set_p is true on i2 and i2 is in between from_ins and i3, then reg_set_between_p will surely be true)? And, shouldn't the record_value_for_reg (XEXP (note, 0), NULL, NULL_RTX); be called in some cases? To me it would make more sense to add the if (reg_set_between_p (...)) break; to the individual cases later, so before if (! (REG_P (XEXP (note, 0)) ? find_regno_note (i3, REG_UNUSED, REGNO (XEXP (note, 0))) : find_reg_note (i3, REG_UNUSED, XEXP (note, 0)))) place = i3; and before PUT_REG_NOTE_KIND (note, REG_DEAD); place = i3; and into the if (from_insn != i3 && i2 && INSN_P (i2) && reg_referenced_p (XEXP (note, 0), PATTERN (i2))) but there just checking if it isn't set in between from_insn and i2 Jakub ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927) 2021-04-18 15:24 ` Jakub Jelinek @ 2021-04-18 17:03 ` Segher Boessenkool 2021-04-19 17:59 ` Segher Boessenkool 0 siblings, 1 reply; 4+ messages in thread From: Segher Boessenkool @ 2021-04-18 17:03 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches Hi! On Sun, Apr 18, 2021 at 05:24:50PM +0200, Jakub Jelinek wrote: > On Sun, Apr 18, 2021 at 03:03:07PM +0000, Segher Boessenkool wrote: > > If the register named in an existing REG_UNUSED note dies somewhere > > between where the note used to be and I3, we should just drop it. > > > > 2021-04-21 Segher Boessenkool <segher@kernel.crashing.org> > > > > PR rtl-optimization/99927 > > * combine.c (distribute_notes) [REG_UNUSED]: If the register already > > is dead, just drop it. > > --- > > > > Committed to trunk. This will need backports to all open branches. > > Thanks for working on this. Just some nits but note that I don't know much > about the combiner... > > > gcc/combine.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/gcc/combine.c b/gcc/combine.c > > index 9063a07bd009..62bf4aeaabae 100644 > > --- a/gcc/combine.c > > +++ b/gcc/combine.c > > @@ -14366,6 +14366,11 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2, > > we keep notes from i2 or i1 if they will turn into REG_DEAD > > notes. */ > > > > + /* If this register is set or clobbered between FROM_INSN and I3, > > + we should not create a note for it. */ > > + if (reg_set_between_p (XEXP (note, 0), from_insn, i3)) > > + break; > > + > > /* If this register is set or clobbered in I3, put the note there > > unless there is one already. */ > > if (reg_set_p (XEXP (note, 0), PATTERN (i3))) > > Doesn't this make the > if (from_insn != i3 && i2 && INSN_P (i2) > && reg_referenced_p (XEXP (note, 0), PATTERN (i2))) > { > if (!reg_set_p (XEXP (note, 0), PATTERN (i2))) > PUT_REG_NOTE_KIND (note, REG_DEAD); > if (! (REG_P (XEXP (note, 0)) > ? find_regno_note (i2, REG_NOTE_KIND (note), > REGNO (XEXP (note, 0))) > : find_reg_note (i2, REG_NOTE_KIND (note), > XEXP (note, 0)))) > place = i2; > } > case unreachable (the reg_set_p stuff at least; I mean if > reg_set_p is true on i2 and i2 is in between from_ins and i3, then > reg_set_between_p will surely be true)? reg_set_between_p is exclusive of the endpoints, so say for example from_insn is i2, and that is where the set is, then your quoted code can fire, while the new code doesn't normally. > And, shouldn't the > record_value_for_reg (XEXP (note, 0), NULL, NULL_RTX); > be called in some cases? I don't see why? We don't remove any clobber or set, just the REG_UNUSED note? > To me it would make more sense to add the if (reg_set_between_p (...)) break; > to the individual cases later, so before > if (! (REG_P (XEXP (note, 0)) > ? find_regno_note (i3, REG_UNUSED, REGNO (XEXP (note, 0))) > : find_reg_note (i3, REG_UNUSED, XEXP (note, 0)))) > place = i3; > and before > PUT_REG_NOTE_KIND (note, REG_DEAD); > place = i3; > and into the > if (from_insn != i3 && i2 && INSN_P (i2) > && reg_referenced_p (XEXP (note, 0), PATTERN (i2))) > but there just checking if it isn't set in between from_insn and i2 But the REG_UNUSED note should just be dropped in all these cases, so it is much simpler code to do it like this. Or am I missing something? Ideally we can revamp all of this to just use DF directly, instead of using notes etc. for it, but that is hard to do. My plan is to attack this from the other direction first: replace reg_stat with something that works better, is more modern, much simpler, much less maintenace, and as a bonus can be used everywhere, not just in combine. We'll see how well that works :-) Segher ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927) 2021-04-18 17:03 ` Segher Boessenkool @ 2021-04-19 17:59 ` Segher Boessenkool 0 siblings, 0 replies; 4+ messages in thread From: Segher Boessenkool @ 2021-04-19 17:59 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On Sun, Apr 18, 2021 at 12:03:39PM -0500, Segher Boessenkool wrote: > On Sun, Apr 18, 2021 at 05:24:50PM +0200, Jakub Jelinek wrote: > > On Sun, Apr 18, 2021 at 03:03:07PM +0000, Segher Boessenkool wrote: > > > If the register named in an existing REG_UNUSED note dies somewhere > > > between where the note used to be and I3, we should just drop it. > > And, shouldn't the > > record_value_for_reg (XEXP (note, 0), NULL, NULL_RTX); > > be called in some cases? > > I don't see why? We don't remove any clobber or set, just the REG_UNUSED > note? > > > To me it would make more sense to add the if (reg_set_between_p (...)) break; > > to the individual cases later, so before > > if (! (REG_P (XEXP (note, 0)) > > ? find_regno_note (i3, REG_UNUSED, REGNO (XEXP (note, 0))) > > : find_reg_note (i3, REG_UNUSED, XEXP (note, 0)))) > > place = i3; > > and before > > PUT_REG_NOTE_KIND (note, REG_DEAD); > > place = i3; > > and into the > > if (from_insn != i3 && i2 && INSN_P (i2) > > && reg_referenced_p (XEXP (note, 0), PATTERN (i2))) > > but there just checking if it isn't set in between from_insn and i2 > > But the REG_UNUSED note should just be dropped in all these cases, so it > is much simpler code to do it like this. Or am I missing something? I have now tested this on kernel builds on all supported archs (and variations; 31 builds, mostly defconfigs, some bigger). The patch changed generated code in no single place, so we're good probably :-) Segher ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-19 18:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-18 15:03 [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927) Segher Boessenkool 2021-04-18 15:24 ` Jakub Jelinek 2021-04-18 17:03 ` Segher Boessenkool 2021-04-19 17:59 ` Segher Boessenkool
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).