Bernd Schmidt <[email protected]> writes:
> On 07/07/11 16:34, Richard Sandiford wrote:
>> Is JUMP_LABEL ever null after this change? (In fully-complete rtl
>> sequences, I mean.) It looked like some of the null checks in the
>> patch might not be necessary any more.
>
> It turns out that computed jumps can have a NULL JUMP_LABEL, and so can
> JUMP_INSNs holding ADDR_VECs.
Bleh. Thanks for checking.
> +/* A wrapper around next_active_insn which takes care to return ret_rtx
> + unchanged. */
> +
> +static rtx
> +active_insn_after (rtx insn)
> +{
> + if (ANY_RETURN_P (insn))
> + return insn;
> + return next_active_insn (insn);
> +}
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?
It wasn't clear to me whether this should return null instead of "insn"
for the ANY_RETURN_P code. In things like:
insn_at_target = active_insn_after (target_label);
it introduces a new "INSN_P or RETURN" rtx choice, rather than the
"label or RETURN" choice seen in JUMP_LABELs. So it might seem at a
glance that PATTERN could be directly applied to a nonnull insn_at_target,
whereas you actually need to test ANY_RETURN_P first.
But the existing code seems inconsistent. Sometimes it passes
JUMP_LABELs directly to functions like own_thread_p, whereas sometimes
it passes the first active insn instead. So if you returned null here,
you'd probably have three-way "null or RETURN or LABEL" checks where you
otherwise wouldn't.
All in all, I agree it's probably better this way.
> @@ -921,7 +933,7 @@ rare_destination (rtx insn)
> int jump_count = 0;
> rtx next;
>
> - 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, as you did in follow_jumps and
skip_consecutive_labels.
> Index: gcc/jump.c
> ===================================================================
> --- gcc/jump.c (revision 176230)
> +++ gcc/jump.c (working copy)
> @@ -1217,7 +1217,7 @@ delete_related_insns (rtx insn)
> /* If deleting a jump, decrement the count of the label,
> and delete the label if it is now unused. */
>
> - if (JUMP_P (insn) && JUMP_LABEL (insn))
> + if (JUMP_P (insn) && !ANY_RETURN_P (JUMP_LABEL (insn)))
> {
> rtx lab = JUMP_LABEL (insn), lab_next;
>
Given what you said above, and given that this is a public function,
I think we should keep the null check.
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:
static bool
jump_to_label_p (rtx insn)
{
return JUMP_P (insn) && JUMP_LABEL (insn) && LABEL_P (JUMP_LABEL (insn));
}
And maybe also:
static rtx
jump_target_insn (rtx insn)
{
return jump_to_label_p (insn) ? JUMP_LABEL (insn) : NULL_RTX;
}
It might help avoid the sprinkling of ANY_RETURN_Ps. Just a suggestion
though, not going to insist.
> /* Throughout LOC, redirect OLABEL to NLABEL. Treat null OLABEL or
> NLABEL as a return. Accrue modifications into the change group. */
>
> @@ -1359,37 +1371,19 @@ redirect_exp_1 (rtx *loc, rtx olabel, rt
> int i;
> const char *fmt;
>
> - if (code == LABEL_REF)
> - {
> - if (XEXP (x, 0) == olabel)
> - {
> - rtx n;
> - if (nlabel)
> - n = gen_rtx_LABEL_REF (Pmode, nlabel);
> - else
> - n = ret_rtx;
> -
> - validate_change (insn, loc, n, 1);
> - return;
> - }
> - }
> - else if (code == RETURN && olabel == 0)
> + if ((code == LABEL_REF && XEXP (x, 0) == olabel)
> + || x == olabel)
> {
> - if (nlabel)
> - x = gen_rtx_LABEL_REF (Pmode, nlabel);
> - else
> - x = ret_rtx;
> - if (loc == &PATTERN (insn))
> - x = gen_rtx_SET (VOIDmode, pc_rtx, x);
> - validate_change (insn, loc, x, 1);
> + validate_change (insn, loc, redirect_target (nlabel), 1);
> return;
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. (Then again, it looks like the old code would create
(set (pc) (return)) when "redirecting" a return to a return.
That doesn't seem like a good idea, and it ought to be dead
anyway with the olabel == nlabel shortcuts.)
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);
I realise this doesn't help for PARALLELs though (just as it didn't
for the old code).
> @@ -4126,6 +4129,18 @@ dead_or_predicable (basic_block test_bb,
> }
>
> no_body:
> + if (JUMP_P (BB_END (dest_edge->src)))
> + new_dest_label = JUMP_LABEL (BB_END (dest_edge->src));
> + else if (other_bb != new_dest)
> + {
> + if (new_dest == EXIT_BLOCK_PTR)
> + new_dest_label = ret_rtx;
> + else
> + new_dest_label = block_label (new_dest);
> + }
> + else
> + new_dest_label = NULL_RTX;
> +
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.
> @@ -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.
> @@ -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...
Looks good to me otherwise.
Richard
