From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10314 invoked by alias); 13 Nov 2007 23:31:36 -0000 Received: (qmail 10290 invoked by uid 22791); 13 Nov 2007 23:31:32 -0000 X-Spam-Check-By: sourceware.org Received: from nic2.axis.se (HELO krynn.se.axis.com) (193.13.178.10) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 13 Nov 2007 23:31:28 +0000 Received: from ignucius.se.axis.com (ignucius.se.axis.com [10.83.5.18]) by krynn.se.axis.com (8.13.4/8.13.4/Debian-3sarge3) with ESMTP id lADNVPuC005115; Wed, 14 Nov 2007 00:31:25 +0100 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 lADNVObH001851; Wed, 14 Nov 2007 00:31:24 +0100 Received: (from hp@localhost) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) id lADNVEcY001847; Wed, 14 Nov 2007 00:31:14 +0100 Date: Wed, 14 Nov 2007 00:24:00 -0000 Message-Id: <200711132331.lADNVEcY001847@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-11/txt/msg00761.txt.bz2 > From: Eric Botcazou > Date: Sun, 9 Sep 2007 11:00:37 +0200 (I replied to this your first comment rather than your last comment, to fit better with the patch. Sorry again for the delay.) > > +++ gcc/reload.c (working copy) > > @@ -4103,13 +4103,18 @@ find_reloads (rtx insn, int replace, int > 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? This and related comments are fixed in the patch for label_is_jump_target_p below. > Same question: what about operands of REG_LABEL_TARGET notes? Ditto. (reorg.c) > > > @@ -2736,14 +2737,40 @@ fill_slots_from_thread (rtx insn, rtx co > > + 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). I meant JUMP_LABEL (trial), doh. See patch. A bit odd that *none* of mips, mips64, cris and sh (all having branch slots) hit this bug in testing but then again reorg.c does not lack corner cases. (jump.c) > > @@ -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? This code rarely hits these days (post DF), if ever. I thought it was just about necessary for targets with branch-target registers for conditional branches like sh64 (unique in this aspect AFAICT) but that target doesn't split the branch-target-register-load from the branch until register allocation and by that time, JUMP_LABEL is safely set. I instrumented the code with a gcc_unreachable, and this code only hits for gcc.c-torture/compile/920501-7.c (for most targets). If you prefer, I can remove this code. The situation may of course change if more passes start calling mark_all_labels. > > > @@ -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. See patch. (gcse.c) > > @@ -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. Quite so. Conditional removed in the patch. I experimented with replacing it with an gcc_assert (reg_mentioned_p (XEXP (x, 0), insn)), and also a corresponding one in mark_jump_label_1, but the latter hits for arm/thumb when loading a label from the constant pool and also for the load-label-feed-jump code above (doh). Anyway, for 130081 there was only one caller of the static function add_label_notes, so an assert didn't seem very necessary. > > > Index: gcc/sched-rgn.c > Why did you drop the check on NEXT_INSN altogether? (Not yet fixed; may "only" affect optimization. I'm on it.) Please consider this patch for all but the last issue, built and tested natively for x86_64-unknown-linux-gnu (without multilibs due to lack of 32-bit libs) and regtested using btest-gcc.sh (with fortran as applicable with my BTEST_GCC_EXTRA_TESTLOGS patch) for cris-elf mmix-knuth-mmixware sh64-elf (no fortran; 130081 ICEd building libgfortran/generated/matmul_r4.c) arm-elf v850-elf sh-elf (no fortran; ICE for generated/_sign_r8.F90) mips-elf mips64-elf Ok to commit? gcc: * rtlanal.c (label_is_jump_target_p): Return true for a matching REG_LABEL_TARGET. * reorg.c (fill_slots_from_thread): Correct last change to use NULL_RTX, not NULL. Outside of REG_NOTES loop, increase and decrease LABEL_NUSES for JUMP_LABEL (trial), not XEXP (note, 0). * jump.c (mark_jump_label_1): Add comment for last change regarding JUMP_LABEL setting. * gcse.c (add_label_notes): Remove conditional that the label is mentioned in insn before adding regnote. Index: rtlanal.c =================================================================== --- rtlanal.c (revision 130159) +++ rtlanal.c (working copy) @@ -3434,6 +3434,9 @@ label_is_jump_target_p (const_rtx label, return true; } + if (find_reg_note (jump_insn, REG_LABEL_TARGET, label)) + return true; + return false; } Index: reorg.c =================================================================== --- reorg.c (revision 130159) +++ reorg.c (working copy) @@ -2740,7 +2740,7 @@ fill_slots_from_thread (rtx insn, rtx co temporarily increment the use count on any referenced label lest it be deleted by delete_related_insns. */ for (note = REG_NOTES (trial); - note != NULL; + note != NULL_RTX; note = XEXP (note, 1)) if (REG_NOTE_KIND (note) == REG_LABEL_OPERAND || REG_NOTE_KIND (note) == REG_LABEL_TARGET) @@ -2754,12 +2754,12 @@ fill_slots_from_thread (rtx insn, rtx co == REG_LABEL_OPERAND); } if (JUMP_P (trial) && JUMP_LABEL (trial)) - LABEL_NUSES (XEXP (note, 0))++; + LABEL_NUSES (JUMP_LABEL (trial))++; delete_related_insns (trial); for (note = REG_NOTES (trial); - note != NULL; + note != NULL_RTX; note = XEXP (note, 1)) if (REG_NOTE_KIND (note) == REG_LABEL_OPERAND || REG_NOTE_KIND (note) == REG_LABEL_TARGET) @@ -2773,7 +2773,7 @@ fill_slots_from_thread (rtx insn, rtx co == REG_LABEL_OPERAND); } if (JUMP_P (trial) && JUMP_LABEL (trial)) - LABEL_NUSES (XEXP (note, 0))--; + LABEL_NUSES (JUMP_LABEL (trial))--; } else new_thread = next_active_insn (trial); Index: jump.c =================================================================== --- jump.c (revision 130159) +++ jump.c (working copy) @@ -1051,6 +1051,9 @@ mark_jump_label_1 (rtx x, rtx insn, bool if (insn) { if (is_target + /* Do not change a previous setting of JUMP_LABEL. If the + JUMP_LABEL slot is occupied by a different label, + create a note for this label. */ && (JUMP_LABEL (insn) == NULL || JUMP_LABEL (insn) == label)) JUMP_LABEL (insn) = label; else Index: gcse.c =================================================================== --- gcse.c (revision 130159) +++ gcse.c (working copy) @@ -4613,18 +4613,16 @@ 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). */ - 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))++; - } + /* 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; } brgds, H-P