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

[Jakub Jelinek <jakub@redhat.com>]

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