This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA:] Split REG_LABEL into REG_LABEL_TARGET and REG_LABEL_OPERAND


> From: Eric Botcazou <ebotcazou@libertysurf.fr>
> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]