From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17879 invoked by alias); 13 Sep 2007 02:03:30 -0000 Received: (qmail 17698 invoked by uid 22791); 13 Sep 2007 02:03:26 -0000 X-Spam-Check-By: sourceware.org Received: from miranda.se.axis.com (HELO miranda.se.axis.com) (193.13.178.8) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 13 Sep 2007 02:03:22 +0000 Received: from ignucius.se.axis.com (ignucius.se.axis.com [10.83.5.18]) by miranda.se.axis.com (8.13.4/8.13.4/Debian-3sarge3) with ESMTP id l8D23H1S006659; Thu, 13 Sep 2007 04:03:17 +0200 Received: from ignucius.se.axis.com (localhost [127.0.0.1]) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) with ESMTP id l8D23HdA010542; Thu, 13 Sep 2007 04:03:17 +0200 Received: (from hp@localhost) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) id l8D236jg010538; Thu, 13 Sep 2007 04:03:06 +0200 Date: Thu, 13 Sep 2007 03:17:00 -0000 Message-Id: <200709130203.l8D236jg010538@ignucius.se.axis.com> From: Hans-Peter Nilsson To: ebotcazou@libertysurf.fr CC: hans-peter.nilsson@axis.com, gcc-patches@gcc.gnu.org In-reply-to: <200709091100.37734.ebotcazou@libertysurf.fr> (message from Eric Botcazou on Sun, 9 Sep 2007 11:00:37 +0200) Subject: Re: [RFA:] Split REG_LABEL into REG_LABEL_TARGET and REG_LABEL_OPERAND 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: 2007-09/txt/msg01148.txt.bz2 > From: Eric Botcazou > Date: Sun, 9 Sep 2007 11:00:37 +0200 Sorry for not replying sooner. Some of your questions warrant a bit of an investigation (details no longer in the head-cache and then there's the df impact) but here's a partial reply: > > Index: gcc/reload.c > > =================================================================== > > --- gcc/reload.c (revision 128285) > > +++ gcc/reload.c (working copy) > > @@ -4103,13 +4103,18 @@ find_reloads (rtx insn, int replace, int > > > > *recog_data.operand_loc[i] = substitution; > > > > - /* If we're replacing an operand with a LABEL_REF, we need > > - to make sure that there's a REG_LABEL note attached to > > + /* If we're replacing an operand with a LABEL_REF, we need to > > + make sure that there's a REG_LABEL_OPERAND note attached to > > this instruction. */ > > - if (!JUMP_P (insn) > > - && GET_CODE (substitution) == LABEL_REF > > - && !find_reg_note (insn, REG_LABEL, XEXP (substitution, 0))) > > - REG_NOTES (insn) = gen_rtx_INSN_LIST (REG_LABEL, > > + if (GET_CODE (substitution) == LABEL_REF > > + && !find_reg_note (insn, REG_LABEL_OPERAND, > > + XEXP (substitution, 0)) > > + /* For a JUMP_P, if it was a branch target it must have > > + already been recorded as such. */ > > + && (!JUMP_P (insn) > > + || !label_is_jump_target_p (XEXP (substitution, 0), > > + insn))) > > + REG_NOTES (insn) = gen_rtx_INSN_LIST (REG_LABEL_OPERAND, > > XEXP (substitution, 0), > > REG_NOTES (insn)); > > } > > You're not updating label_is_jump_target_p to take into account the operands > of REG_LABEL_TARGET notes, so can't you be creating REG_LABEL_OPERAND notes > for them? (I assume you mean s/can't/shouldn't/ and "REG_LABEL_TARGET notes for them". If not, please rephrase the question.) To trig that case, a target is needed that can jump to more than one location in the same insn (e.g. a conditional branch that doesn't fall through) but which isn't a *simple* tablejump, because otherwise label_is_jump_target_p fits as-is. (Hm, the CRIS "*casesi_adds_w" insn seems to match that somewhat; the label for the jump-table is possibly dangling unless tablejumps are explicitly handled... need to investigate.) If there's a problem, the remedy would be to update label_is_jump_target_p to be less simplified than its heading says; to handle REG_LABEL_TARGET notes. (I *think* code elsewhere will fail for non-tablejump insns with more than one branch target, but anyway consistency is a reason to update it.) The code here just needs to cope with updating REG_LABEL_OPERAND notes; labels that aren't trivially jumped to (see original, "-"-marked code). All REG_LABEL_TARGET notes are added long before reload. > > @@ -6123,17 +6128,15 @@ subst_reloads (rtx insn) > > } > > #endif /* DEBUG_RELOAD */ > > > > - /* If we're replacing a LABEL_REF with a register, add a > > - REG_LABEL note to indicate to flow which label this > > + /* If we're replacing a LABEL_REF with a register, there must > > + already be an indication (to e.g. flow) which label this > > register refers to. */ > > - if (GET_CODE (*r->where) == LABEL_REF > > - && JUMP_P (insn)) > > - { > > - REG_NOTES (insn) = gen_rtx_INSN_LIST (REG_LABEL, > > - XEXP (*r->where, 0), > > - REG_NOTES (insn)); > > - JUMP_LABEL (insn) = XEXP (*r->where, 0); > > - } > > + gcc_assert (GET_CODE (*r->where) != LABEL_REF > > + || !JUMP_P (insn) > > + || find_reg_note (insn, > > + REG_LABEL_OPERAND, > > + XEXP (*r->where, 0)) > > + || label_is_jump_target_p (XEXP (*r->where, 0), insn)); > > > > /* Encapsulate RELOADREG so its machine mode matches what > > used to be there. Note that gen_lowpart_common will > > Same question: what about operands of REG_LABEL_TARGET notes? They've already been generated when we come to reload; for a simple un/conditional jump the label sticks to the JUMP_LABEL field. The semantics are changed to be sticky, so they don't need to be added again. Note the old code adding them to both the JUMP_LABEL field and as a REG_LABEL note! (For this code and the one above; since you have to ask, this code needs a comment to the effect of my reply.) > > @@ -2736,14 +2737,40 @@ fill_slots_from_thread (rtx insn, rtx co > > /* We are moving this insn, not deleting it. We must > > temporarily increment the use count on any referenced > > label lest it be deleted by delete_related_insns. */ > > - note = find_reg_note (trial, REG_LABEL, 0); > > - /* REG_LABEL could be NOTE_INSN_DELETED_LABEL too. */ > > - if (note && LABEL_P (XEXP (note, 0))) > > + for (note = REG_NOTES (trial); > > + note != NULL; > > + note = XEXP (note, 1)) > > + if (REG_NOTE_KIND (note) == REG_LABEL_OPERAND > > + || REG_NOTE_KIND (note) == REG_LABEL_TARGET) > > + { > > + /* REG_LABEL_OPERAND could be > > + NOTE_INSN_DELETED_LABEL too. */ > > + if (LABEL_P (XEXP (note, 0))) > > + LABEL_NUSES (XEXP (note, 0))++; > > + else > > + gcc_assert (REG_NOTE_KIND (note) > > + == REG_LABEL_OPERAND); > > + } > > + if (JUMP_P (trial) && JUMP_LABEL (trial)) > > LABEL_NUSES (XEXP (note, 0))++; > > AFAICS note == NULL at this point (and it should be NULL_RTX instead of NULL). > ... > Likewise. Yes... typo: they should just move inside the closing bracket on the previous line. I'm curious this didn't trig a SEGV in my testing. Investigating. > > static int > > check_for_label_ref (rtx *rtl, void *data) > > { > > rtx insn = (rtx) data; > > > > - /* If this insn uses a LABEL_REF and there isn't a REG_LABEL note for > > it, - we must rerun jump since it needs to place the note. If this is > > a - LABEL_REF for a CODE_LABEL that isn't in the insn chain, don't do > > this - since no REG_LABEL will be added. */ > > + /* If this insn uses a LABEL_REF and there isn't a REG_LABEL_OPERAND > > + note for it, we must rerun jump since it needs to place the note. If > > + this is a LABEL_REF for a CODE_LABEL that isn't in the insn chain, > > + don't do this since no REG_LABEL_OPERAND will be added. */ > > return (GET_CODE (*rtl) == LABEL_REF > > && ! LABEL_REF_NONLOCAL_P (*rtl) > > + && (!JUMP_P (insn) > > + || !label_is_jump_target_p (XEXP (*rtl, 0), insn)) > > && LABEL_P (XEXP (*rtl, 0)) > > && INSN_UID (XEXP (*rtl, 0)) != 0 > > - && ! find_reg_note (insn, REG_LABEL, XEXP (*rtl, 0))); > > + && ! find_reg_note (insn, REG_LABEL_OPERAND, XEXP (*rtl, 0))); > > What about operands of REG_LABEL_TARGET notes? They aren't supposed to (must not) be handled here. This code is only for REG_LABEL_OPERAND notes needing to be updated; non-jump-insns moved around without notes being updated. Same comment about label_is_jump_target_p maybe being too simple applies. (Since you have to ask, this function needs a comment to this effect.) > > @@ -172,34 +186,69 @@ static void > > mark_all_labels (rtx f) > > { > > rtx insn; > > + rtx prev_nonjump_insn = NULL; > > > > for (insn = f; insn; insn = NEXT_INSN (insn)) > > if (INSN_P (insn)) > > { > > mark_jump_label (PATTERN (insn), insn, 0); > > - if (! INSN_DELETED_P (insn) && JUMP_P (insn)) > > + > > + /* If the previous non-jump insn sets something to a label, > > + something that this jump insn uses, make that label the primary > > + target of this insn if we don't yet have any. That previous > > + insn must be a single_set and not refer to more than one label. > > + The jump insn must not refer to other labels as jump targets > > + and must be a plain (set (pc) ...), maybe in a parallel, and > > + may refer to the item being set only directly or as one of the > > + arms in an IF_THEN_ELSE. */ > > + if (! INSN_DELETED_P (insn) > > + && JUMP_P (insn) > > + && JUMP_LABEL (insn) == NULL) > > Why do you need to do this? Is it an optimization? It's the core machinery for targets with branch-target registers, to have the label recorded as a branch target before further optimizations moves the set and use of that register apart. Otherwise, those branches will all be treated as computed jumps. Hm, we'll still have the REG_LABEL_OPERAND note on the register load, but then again, so we did before too. I'll check that this still happens and add a comment actually mentioning "branch-target register"! > > > @@ -976,17 +1050,21 @@ mark_jump_label (rtx x, rtx insn, int in > > > > if (insn) > > { > > - if (JUMP_P (insn)) > > + if (is_target > > + && (JUMP_LABEL (insn) == NULL || JUMP_LABEL (insn) == label)) > > JUMP_LABEL (insn) = label; > > Please add a small comment, one can easily think there is a typo in the second > part of the disjunction: JUMP_LABEL (insn) != label. Will do. > > @@ -4608,10 +4609,18 @@ add_label_notes (rtx x, rtx insn) > > We no longer ignore such label references (see LABEL_REF handling in > > mark_jump_label for additional information). */ > > > > - REG_NOTES (insn) = gen_rtx_INSN_LIST (REG_LABEL, XEXP (x, 0), > > - REG_NOTES (insn)); > > - if (LABEL_P (XEXP (x, 0))) > > - LABEL_NUSES (XEXP (x, 0))++; > > + if (reg_mentioned_p (XEXP (x, 0), insn)) > > + { > > + /* There's no reason for current users to emit jump-insns > > + with such a LABEL_REF, so we don't have to handle > > + REG_LABEL_TARGET notes. */ > > + gcc_assert (!JUMP_P (insn)); > > + REG_NOTES (insn) > > + = gen_rtx_INSN_LIST (REG_LABEL_OPERAND, XEXP (x, 0), > > + REG_NOTES (insn)); > > + if (LABEL_P (XEXP (x, 0))) > > + LABEL_NUSES (XEXP (x, 0))++; > > + } > > return; > > } > > "Put in line with jump.c copy by only adding notes for labels actually > referenced in the insn" but AFAICS mark_jump_label doesn't do that. Uhm, it does; it iterates on the insn, and when it finds LABEL_REFs, it adds unique REG_LABEL_OPERAND notes (and sets the JUMP_LABEL field and emits REG_LABEL_TARGET notes, but they weren't the issue here). It no longer emit duplicates. Rephrase suggested? Perhaps just "Make sure not to add duplicate REG_LABEL_OPERAND notes" and referring to consistency by adding a comment in the add_label_notes code? > > Index: gcc/sched-rgn.c > > =================================================================== > > --- gcc/sched-rgn.c (revision 128285) > > +++ gcc/sched-rgn.c (working copy) > > @@ -315,24 +315,20 @@ is_cfg_nonregular (void) > > if (current_function_has_exception_handlers ()) > > return 1; > > > > - /* If we have non-jumping insns which refer to labels, then we consider > > - the cfg not well structured. */ > > + /* If we have insns which refer to labels as non-jumped-to operands, > > + then we consider the cfg not well structured. */ > > FOR_EACH_BB (b) > > FOR_BB_INSNS (b, insn) > > { > > - /* Check for labels referred by non-jump insns. */ > > - if (NONJUMP_INSN_P (insn) || CALL_P (insn)) > > - { > > - rtx note = find_reg_note (insn, REG_LABEL, NULL_RTX); > > - if (note > > - && ! (JUMP_P (NEXT_INSN (insn)) > > - && find_reg_note (NEXT_INSN (insn), REG_LABEL, > > - XEXP (note, 0)))) > > - return 1; > > - } > > + /* Check for labels referred to but (at least not directly) as > > + jump targets. */ > > + if (INSN_P (insn) > > + && find_reg_note (insn, REG_LABEL_OPERAND, NULL_RTX)) > > + return 1; > > + > > /* If this function has a computed jump, then we consider the cfg > > not well structured. */ > > - else if (JUMP_P (insn) && computed_jump_p (insn)) > > + if (JUMP_P (insn) && computed_jump_p (insn)) > > return 1; > > } > > Why did you drop the check on NEXT_INSN altogether? Because that old code was sloppily only trying to deal with the immediately consecutive (no notes or anything in-between!) insn pair (set branch-target-reg label) (set pc branch-target-reg) as a non-computed jump (the whole REG_LABEL ambiguity). Hm, the new code will still have a REG_LABEL_OPERAND on the (set branch-target-reg label) so that'll always be seen as is_cfg_nonregular. Will investigate the impact and how to merge identifiable set/jump pairs as non-computed. I hope REG_DEAD notes are present here... brgds, H-P