On 6/2/23 02:46, Georg-Johann Lay wrote:
There is the following bug in postreload that can be traced back
to v5 at least:
In postreload.cc::reload_cse_move2add() there is a loop over all
insns. If it encounters a SET, the next insn is analyzed if it
is a single_set.
After next has been analyzed, it continues with
if (success)
delete_insn (insn);
changed |= success;
insn = next; // This effectively skips analysis of next.
move2add_record_mode (reg);
reg_offset[regno]
= trunc_int_for_mode (added_offset + base_offset,
mode);
continue; // for continues with insn = NEXT_INSN (insn).
So it records the effect of next, but not the clobbers that
next might have. This is a problem if next clobbers a GPR
like it can happen for avr. What then can happen is that in a
later round, it may use a value from a (partially) clobbered reg.
The patch records the effects of potential clobbers.
Bootstrapped and reg-tested on x86_64. Also tested on avr where
the bug popped up. The testcase discriminates on avr, and for now
I am not aware of any other target that's affected by the bug.
The change is not intrusive and fixes wrong code, so I'd like
to backport it.
Ok to apply?
Johann
rtl-optimization/101188: Don't bypass clobbers of some insns that are
optimized or are optimization candidates.
gcc/
PR rtl-optimization/101188
* postreload.cc (reload_cse_move2add): Record clobbers of next
insn using move2add_note_store.
gcc/testsuite/
PR rtl-optimization/101188
* gcc.c-torture/execute/pr101188.c: New test.
If I understand the code correctly, isn't the core of the problem that
we "continue" rather than executing the rest of the code in the loop.
In particular the continue bypasses this chunk of code:
for (note = REG_NOTES (insn); note; note = XEXP (note, 1))
{
if (REG_NOTE_KIND (note) == REG_INC
&& REG_P (XEXP (note, 0)))
{
/* Reset the information about this register. */
int regno = REGNO (XEXP (note, 0));
if (regno < FIRST_PSEUDO_REGISTER)
{
move2add_record_mode (XEXP (note, 0));
reg_mode[regno] = VOIDmode;
}
}
}
/* There are no REG_INC notes for SP autoinc. */
subrtx_var_iterator::array_type array;
FOR_EACH_SUBRTX_VAR (iter, array, PATTERN (insn), NONCONST)
{
rtx mem = *iter;
if (mem
&& MEM_P (mem)
&& GET_RTX_CLASS (GET_CODE (XEXP (mem, 0))) == RTX_AUTOINC)
{
if (XEXP (XEXP (mem, 0), 0) == stack_pointer_rtx)
reg_mode[STACK_POINTER_REGNUM] = VOIDmode;
}
}
note_stores (insn, move2add_note_store, insn);
Of particular importance for your case would be the note_stores call.
But I could well see other targets needing the search for REG_INC notes
as well as stack pushes.
If I'm right, then wouldn't it be better to factor that blob of code
above into its own function, then use it before the "continue" rather
than implementing a custom can for CLOBBERS?
It also begs the question if the other case immediately above the code I
quoted needs similar adjustment. It doesn't do the insn = next, but it
does bypass the search for autoinc memory references and the note_stores
call.
Jeff
