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

[Hans-Peter Nilsson <hans-peter.nilsson@axis.com>]

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