On 04/09/2013 10:15 AM, Eric Botcazou wrote:
Yes, I agree that it's quite convoluted but, as Steven already said, rewriting
it to use a more modern framework is hard because of SEQUENCEs and, frankly, I
have neither the real incentive nor the time to start such an endeavor.
I understand completely.
Instead I can raise the following couple of points that I also ran into with
the private port I'm working on:
1. When fill_simple_delay_slots is scanning backwards to find insns to put
into slots, it calls update_reg_dead_note which collects REG_DEAD notes and
puts them on the insn to be moved. But emit_delay_sequence will later drop
them on the floor. I have one case where preserving such a REG_DEAD note is
correct and makes a difference (slot filled vs empty).
I don't recall ever working on this aspect of reorg. The obvious worry
is that with reorg moving stuff around those notes may not be valid
anymore in the general case.
This is further complicated by the fact that reorg mucks up the CFG
enough that correct, incremental updates of life information might be
exceedingly hard.
2. In resource.c, when mark_target_live_regs is doing its liveness analysis,
it extracts the insns wrapped in USEs by reorg.c:
/* If this insn is a USE made by update_block, we care about the
underlying insn. */
if (code == INSN && GET_CODE (PATTERN (insn)) == USE
&& INSN_P (XEXP (PATTERN (insn), 0)))
real_insn = XEXP (PATTERN (insn), 0);
and proceeds with the liveness analysis without further ado. Now, the story
is different in find_dead_or_set_registers, which does:
Just a note, there's a formatting error in this code. The real_insn =
statement is indented too far. Feel free to fix that :-)
This looks like the safe/conservative thing for this code. We're
marking when regs become live. Marking something live too early doesn't
hurt here (from a correctness standpoint) as far as I can tell.
Note that deaths are deferred until the next label.
if (GET_CODE (PATTERN (insn)) == USE)
{
/* If INSN is a USE made by update_block, we care about the
underlying insn. Any registers set by the underlying insn
are live since the insn is being done somewhere else. */
if (INSN_P (XEXP (PATTERN (insn), 0)))
mark_set_resources (XEXP (PATTERN (insn), 0), res, 0,
MARK_SRC_DEST_CALL);
and thus pessimizes the analysis by making registers unnecessary live. Again
I have one case where not pessimizing is correct and makes a difference.
Seems reasonable. This might just be an oversight by the original
author. Effectively we're going to start marking the referenced
resources with your patch. We still defer killing as far as I can tell.
Experimental patch attached, clean on the C testsuite for the private port.
What do you think?
If you want to run with it, I won't object. My worry would be we might
not see the fallout for a long time as the number of things that have to
fall into place for an observable failure here are very high.
Jeff
