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 <seg...@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
