Fill more delay slots in conditional returns

> Ah, OK, yea, it makes perfect sense now.  Approved.

Thanks.

> If you wanted to get ambitious, rewriting reorg.c to compute and use
> dependency links to drive its candidate selection instead of the insane
> tracking code it uses would be a huge step forward, both in terms of
> cleanliness.  It'd also help compile-time performance; we do a lot of
> work to track resources that would be totally unnecessary.

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.

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).

 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:

	  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.

Experimental patch attached, clean on the C testsuite for the private port.

What do you think?

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