On Mon, Feb 24, 2025 at 10:46 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Wed, Feb 12, 2025 at 1:16 PM Uros Bizjak <ubiz...@gmail.com> wrote:
> >
> > The combine pass is trying to combine:
> >
> > Trying 16, 22, 21 -> 23:
> > 16: r104:QI=flags:CCNO>0
> > 22: {r120:QI=r104:QI^0x1;clobber flags:CC;}
> > REG_UNUSED flags:CC
> > 21: r119:QI=flags:CCNO<=0
> > REG_DEAD flags:CCNO
> > 23: {r110:QI=r119:QI|r120:QI;clobber flags:CC;}
> > REG_DEAD r120:QI
> > REG_DEAD r119:QI
> > REG_UNUSED flags:CC
> >
> > and creates the following two insn sequence:
> >
> > modifying insn i2 22: r104:QI=flags:CCNO>0
> > REG_DEAD flags:CC
> > deferring rescan insn with uid = 22.
> > modifying insn i3 23: r110:QI=flags:CCNO<=0
> > REG_DEAD flags:CC
> > deferring rescan insn with uid = 23.
> >
> > where the REG_DEAD note in i2 is not correct, because the flags
> > register is still referenced in i3. In try_combine() megafunction, we
> > have this part:
> >
> > --cut here--
> > /* Distribute all the LOG_LINKS and REG_NOTES from I1, I2, and I3. */
> > if (i3notes)
> > distribute_notes (i3notes, i3, i3, newi2pat ? i2 : NULL,
> > elim_i2, elim_i1, elim_i0);
> > if (i2notes)
> > distribute_notes (i2notes, i2, i3, newi2pat ? i2 : NULL,
> > elim_i2, elim_i1, elim_i0);
> > if (i1notes)
> > distribute_notes (i1notes, i1, i3, newi2pat ? i2 : NULL,
> > elim_i2, local_elim_i1, local_elim_i0);
> > if (i0notes)
> > distribute_notes (i0notes, i0, i3, newi2pat ? i2 : NULL,
> > elim_i2, elim_i1, local_elim_i0);
> > if (midnotes)
> > distribute_notes (midnotes, NULL, i3, newi2pat ? i2 : NULL,
> > elim_i2, elim_i1, elim_i0);
> > --cut here--
> >
> > where the compiler distributes REG_UNUSED note from i2:
> >
> > 22: {r120:QI=r104:QI^0x1;clobber flags:CC;}
> > REG_UNUSED flags:CC
> >
> > via distribute_notes() using the following:
> >
> > --cut here--
> > /* Otherwise, if this register is now referenced in i2
> > then the register used to be modified in one of the
> > original insns. If it was i3 (say, in an unused
> > parallel), it's now completely gone, so the note can
> > be discarded. But if it was modified in i2, i1 or i0
> > and we still reference it in i2, then we're
> > referencing the previous value, and since the
> > register was modified and REG_UNUSED, we know that
> > the previous value is now dead. So, if we only
> > reference the register in i2, we change the note to
> > REG_DEAD, to reflect the previous value. However, if
> > we're also setting or clobbering the register as
> > scratch, we know (because the register was not
> > referenced in i3) that it's unused, just as it was
> > unused before, and we place the note in i2. */
> > 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;
> > }
> > --cut here--
> >
> > However, the flags register is not UNUSED (or DEAD), because it is
> > used in i3. The proposed solution is to remove the REG_UNUSED note
> > from i2 when the register is also mentioned in i3.
>
> But there is
>
> /* Otherwise, if this register is used by I3, then this register
> now dies here, so we must put a REG_DEAD note here unless there
> is one already. */
> else if (reg_referenced_p (XEXP (note, 0), PATTERN (i3))
> && ! (REG_P (XEXP (note, 0))
> ? find_regno_note (i3, REG_DEAD,
> REGNO (XEXP (note, 0)))
> : find_reg_note (i3, REG_DEAD, XEXP (note, 0))))
> {
> PUT_REG_NOTE_KIND (note, REG_DEAD);
> place = i3;
> }
>
> which of course isn't taken as there is a NOTE in i3 already. Still the
> note is supposed to be moved there (just not emitted, it's already there.
> So a better fix is to refactor the above to
>
> /* Otherwise, if this register is used by I3, then this register
> now dies here, so we must put a REG_DEAD note here unless there
> is one already. */
> else if (reg_referenced_p (XEXP (note, 0), PATTERN (i3))
> {
> if (! (REG_P (XEXP (note, 0))
> ? find_regno_note (i3, REG_DEAD,
> REGNO (XEXP (note, 0)))
> : find_reg_note (i3, REG_DEAD, XEXP (note, 0))))
> {
> PUT_REG_NOTE_KIND (note, REG_DEAD);
> place = i3;
> }
> }
>
> ? At least the else { case seems to assume the reg isn't refernced in i3.
> The comment wording might also need an update of course.
Yes, this revision works as well. I'll prepare a v2 patch.
Thanks,
Uros.
