On Wed, Feb 12, 2025 at 1:16 PM Uros Bizjak <[email protected]> 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.
Richard.
> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> OK for master and eventual backports?
>
> Uros.
