[PATCH 4/6] Shrink-wrapping

[Bernd Schmidt]

On 07/21/11 11:52, Richard Sandiford wrote:
> The name "active_insn_after" seems a bit too similar to "next_active_insn"
> for the difference to be obvious.  How about something like
> "first_active_target_insn" instead?

Changed.
>> -  for (; insn; insn = next)
>> +  for (; insn && !ANY_RETURN_P (insn); insn = next)
>>      {
>>        if (NONJUMP_INSN_P (insn) && GET_CODE (PATTERN (insn)) == SEQUENCE)
>>  	insn = XVECEXP (PATTERN (insn), 0, 0);
> 
> Since ANY_RETURN looks for patterns, while this loop iterates over insns,
> I think it'd be more obvious to have:
> 
>   if (insn && ANY_RETURN_P (insn))
>     return 1;
> 
> above the loop instead

That alone wouldn't work since we assign JUMP_LABELs to next. Left alone
for now.

>> --- gcc/jump.c	(revision 176230)
>> +++ gcc/jump.c	(working copy)
>> @@ -1217,7 +1217,7 @@ delete_related_insns (rtx insn)
> 
> Given what you said above, and given that this is a public function,
> I think we should keep the null check.

Changed.
> 
> This pattern came up in reorg.c too, so maybe it would be worth having
> a jump_to_label_p inline function somewhere, such as:

Done. Only has two uses for now though; reorg.c uses different patterns
mostly.

> It looks like the old code tried to allow returns to be redirected
> to a label -- (return) to (set (pc) (label_ref)) -- whereas the new
> code doesn't. [...]
> 
> How about:
> 
>       x = redirect_target (nlabel);
>       if (GET_CODE (x) == LABEL_REF && loc == &PATTERN (insn))
> 	x = gen_rtx_SET (VOIDmode, pc_rtx, x);
>       validate_change (insn, loc, x, 1);

Changed, although this probably isn't a useful thing to allow (it will
just add one more unnecessary jump to the code?).

[ifcvt changes]
> I found the placement of this code a bit confusing as things stand.
> new_dest_label is only meaningful if other_bb != new_dest, so it seemed
> like something that should directly replace the existing new_label
> assignment.  It's OK if it makes the shrink-wrap stuff easier though.

Changed.

>> @@ -1195,6 +1195,9 @@ duplicate_insn_chain (rtx from, rtx to)
>>  	      break;
>>  	    }
>>  	  copy = emit_copy_of_insn_after (insn, get_last_insn ());
>> +	  if (JUMP_P (insn) && JUMP_LABEL (insn) != NULL_RTX
>> +	      && ANY_RETURN_P (JUMP_LABEL (insn)))
>> +	    JUMP_LABEL (copy) = JUMP_LABEL (insn);
> 
> I think this should go in emit_copy_of_insn_after instead.

Here I'd like to avoid modifying the existing code in
emit_copy_of_insn_after if possible. Not sure why it's not copying
JUMP_LABELS, but that's something I'd prefer to investigate at some
other time rather than risk breaking things.

>> @@ -2294,6 +2294,8 @@ create_cfi_notes (void)
>>  	  dwarf2out_frame_debug (insn, false);
>>  	  continue;
>>  	}
>> +      if (GET_CODE (pat) == ADDR_VEC || GET_CODE (pat) == ADDR_DIFF_VEC)
>> +	continue;
>>  
>>        if (GET_CODE (pat) == SEQUENCE)
>>  	{
> 
> rth better approve this bit...

It went away.

New patch below. Retested on i686-linux and mips64-elf. Ok?


Bernd
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: jlabel0726b.diff
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20110728/ee4190c1/attachment.ksh>