[PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927)

[PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927)

[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

Re: [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927)

[Jakub Jelinek]

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

Re: [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927)

[Segher Boessenkool]

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

Re: [PATCH] combine: Don't create REG_UNUSED notes if the reg already died (PR99927)

[Segher Boessenkool]

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