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

[Eric Botcazou <ebotcazou at libertysurf dot fr>]

> 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?

> @@ -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?

> @@ -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).

>  		      delete_related_insns (trial);
>
> -		      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))--;

Likewise.

>  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?

> @@ -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?

> @@ -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.

> @@ -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.

> 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?

-- 
Eric Botcazou