From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6472 invoked by alias); 21 Jul 2011 09:53:39 -0000 Received: (qmail 6463 invoked by uid 22791); 21 Jul 2011 09:53:38 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-fx0-f49.google.com (HELO mail-fx0-f49.google.com) (209.85.161.49) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 21 Jul 2011 09:53:04 +0000 Received: by fxd20 with SMTP id 20so2492678fxd.22 for ; Thu, 21 Jul 2011 02:53:02 -0700 (PDT) Received: by 10.223.52.155 with SMTP id i27mr20898fag.139.1311241982278; Thu, 21 Jul 2011 02:53:02 -0700 (PDT) Received: from richards-thinkpad (gbibp9ph1--blueice3n2.emea.ibm.com [195.212.29.84]) by mx.google.com with ESMTPS id f7sm549628faa.32.2011.07.21.02.53.00 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 21 Jul 2011 02:53:00 -0700 (PDT) From: Richard Sandiford To: Bernd Schmidt Mail-Followup-To: Bernd Schmidt ,GCC Patches , richard.sandiford@linaro.org Cc: GCC Patches Subject: Re: [PATCH 4/6] Shrink-wrapping References: <4D8A0703.9090306@codesourcery.com> <4D8A095C.8050809@codesourcery.com> <4E2766B8.80904@codesourcery.com> Date: Thu, 21 Jul 2011 11:25:00 -0000 In-Reply-To: <4E2766B8.80904@codesourcery.com> (Bernd Schmidt's message of "Thu, 21 Jul 2011 01:37:28 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-07/txt/msg01737.txt.bz2 Bernd Schmidt 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