On 28/07/11 11:35, Bernd Schmidt wrote:
> 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
This causes http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49891
R.
