Re: RFA: redirect_jump_1: fix REG_EQUAL notes

[Joern RENNECKE <joern dot rennecke at st dot com>]

Roger Sayle wrote:

>    
>
> Indeed, I'd like things to be simpler still.  The use of these foo_jump_1
> instructions surrounds the minimal amount of code that needs to be rolled
> back if the substitution isn't recognized.  All of the auxillary data
> structures including JUMP_LABEL, LABEL_NUSES, REG_BR_PROB (branch
> probability notes) are all updated (in ifcvt.c) after the change group.
>
> I can see absolutely no reason why we should handle REG_EQUAL notes
> and REG_BR_PROB notes on a single jump instruction any differently,
> and should therefore modify/update them at the same time.  Preferably,
> once we've commited to making the change after apply_change_group
> succedes.
>
>
> I won't claim that I'm happy about this small amount of duplication,
> but this is the design decision decided upon by RTH when he merged
> part 11 of the condexec functionality into mainline back in April 2000.
> I think following the spirit of those changes and keeping the code
> consistent is almost as important as the small performance overheads
> of your approach.  More generally, I'd like to see uses of validate_change
> where not absolutely necessary, slowly be replaced from the RTL
> optimizers, typically by using simplify_replace_rtx, or start_sequence,
> etc...  In this case, however I agree with RTH's choice of avoiding
> using redirect_jump/invert_jump directly to prevent messing up the
> CFG, until ifcvt confirms that the whole block can be conditionally
> executed.
>  
In my new patch, I have moved the REG_EQUAL note updating to the point
after we go ahead with the optimization.  I have left the use of 
validate_change
in, however.  We are spending most of our time looking for optimizations 
that
we can perform, not actually installing the changes.  And updating the
REG_EQUAL note is only a small part of the latter.  So any compile-time
impact should be neglegible.  In fact, it might even be faster because the
necessary functions should be aleady hot in the cache.  re-using the 
code that
is manipulating the jump PATTERN to manipulate the note makes the code
not only simpler, it also lowers maintenance when we want to start 
supporting
more complex conditions.
  
2005-03-01  J"orn Rennecke <joern.rennecke@st.com>

	* rtl.h (redirect_jump_2): Declare.
	* jump.c (redirect_exp, invert_exp): Delete.
	(invert_exp_1): Take second parameter.  Return value.  Changed caller.
	(redirect_jump_2): New function, broken out of redirect_jump.
	(redirect_jump): Use redirect_jump_1 and redirect_jump_2.
	(invert_jump): Use invert_jump_1 and redirect_jump_2.
	* ifcvt.c (dead_or_predicable): Use redirect_jump_2.

Index: rtl.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/rtl.h,v
retrieving revision 1.536
diff -p -r1.536 rtl.h
*** rtl.h	15 Feb 2005 07:19:51 -0000	1.536
--- rtl.h	1 Mar 2005 19:57:57 -0000
*************** extern int rtx_renumbered_equal_p (rtx, 
*** 1908,1913 ****
--- 1908,1914 ----
  extern int true_regnum (rtx);
  extern unsigned int reg_or_subregno (rtx);
  extern int redirect_jump_1 (rtx, rtx);
+ extern void redirect_jump_2 (rtx, rtx, rtx, int, int);
  extern int redirect_jump (rtx, rtx, int);
  extern void rebuild_jump_labels (rtx);
  extern enum rtx_code reversed_comparison_code (rtx, rtx);
Index: jump.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/jump.c,v
retrieving revision 1.253
diff -p -r1.253 jump.c
*** jump.c	24 Nov 2004 11:32:23 -0000	1.253
--- jump.c	1 Mar 2005 19:57:57 -0000
*************** static void init_label_info (rtx);
*** 67,75 ****
  static void mark_all_labels (rtx);
  static void delete_computation (rtx);
  static void redirect_exp_1 (rtx *, rtx, rtx, rtx);
! static int redirect_exp (rtx, rtx, rtx);
! static void invert_exp_1 (rtx);
! static int invert_exp (rtx);
  static int returnjump_p_1 (rtx *, void *);
  static void delete_prior_computation (rtx, rtx);
  
--- 67,73 ----
  static void mark_all_labels (rtx);
  static void delete_computation (rtx);
  static void redirect_exp_1 (rtx *, rtx, rtx, rtx);
! static int invert_exp_1 (rtx, rtx);
  static int returnjump_p_1 (rtx *, void *);
  static void delete_prior_computation (rtx, rtx);
  
*************** redirect_exp_1 (rtx *loc, rtx olabel, rt
*** 1570,1594 ****
      }
  }
  
- /* Similar, but apply the change group and report success or failure.  */
- 
- static int
- redirect_exp (rtx olabel, rtx nlabel, rtx insn)
- {
-   rtx *loc;
- 
-   if (GET_CODE (PATTERN (insn)) == PARALLEL)
-     loc = &XVECEXP (PATTERN (insn), 0, 0);
-   else
-     loc = &PATTERN (insn);
- 
-   redirect_exp_1 (loc, olabel, nlabel, insn);
-   if (num_validated_changes () == 0)
-     return 0;
- 
-   return apply_change_group ();
- }
- 
  /* Make JUMP go to NLABEL instead of where it jumps now.  Accrue
     the modifications into the change group.  Return false if we did
     not see how to do that.  */
--- 1568,1573 ----
*************** int
*** 1622,1635 ****
  redirect_jump (rtx jump, rtx nlabel, int delete_unused)
  {
    rtx olabel = JUMP_LABEL (jump);
-   rtx note;
  
    if (nlabel == olabel)
      return 1;
  
!   if (! redirect_exp (olabel, nlabel, jump))
      return 0;
  
    JUMP_LABEL (jump) = nlabel;
    if (nlabel)
      ++LABEL_NUSES (nlabel);
--- 1601,1628 ----
  redirect_jump (rtx jump, rtx nlabel, int delete_unused)
  {
    rtx olabel = JUMP_LABEL (jump);
  
    if (nlabel == olabel)
      return 1;
  
!   if (! redirect_jump_1 (jump, nlabel) || ! apply_change_group ())
      return 0;
  
+   redirect_jump_2 (jump, olabel, nlabel, delete_unused, 0);
+   return 1;
+ }
+ 
+ /* Fix up JUMP_LABEL and label ref counts after OLABEL has been replaced with
+    NLABEL in JUMP.  If DELETE_UNUSED is non-negative, copy a
+    NOTE_INSN_FUNCTION_END found after OLABEL to the place after NLABEL.
+    If DELETE_UNUSED is positive, delete related insn to OLABEL if its ref
+    count has dropped to zero.  */
+ void
+ redirect_jump_2 (rtx jump, rtx olabel, rtx nlabel, int delete_unused,
+ 		 int invert)
+ {
+   rtx note;
+ 
    JUMP_LABEL (jump) = nlabel;
    if (nlabel)
      ++LABEL_NUSES (nlabel);
*************** redirect_jump (rtx jump, rtx nlabel, int
*** 1637,1660 ****
    /* Update labels in any REG_EQUAL note.  */
    if ((note = find_reg_note (jump, REG_EQUAL, NULL_RTX)) != NULL_RTX)
      {
!       if (nlabel && olabel)
  	{
! 	  rtx dest = XEXP (note, 0);
! 
! 	  if (GET_CODE (dest) == IF_THEN_ELSE)
! 	    {
! 	      if (GET_CODE (XEXP (dest, 1)) == LABEL_REF
! 		  && XEXP (XEXP (dest, 1), 0) == olabel)
! 		XEXP (XEXP (dest, 1), 0) = nlabel;
! 	      if (GET_CODE (XEXP (dest, 2)) == LABEL_REF
! 		  && XEXP (XEXP (dest, 2), 0) == olabel)
! 		XEXP (XEXP (dest, 2), 0) = nlabel;
! 	    }
! 	  else
! 	    remove_note (jump, note);
  	}
-       else
-         remove_note (jump, note);
      }
  
    /* If we're eliding the jump over exception cleanups at the end of a
--- 1630,1642 ----
    /* Update labels in any REG_EQUAL note.  */
    if ((note = find_reg_note (jump, REG_EQUAL, NULL_RTX)) != NULL_RTX)
      {
!       if (!nlabel || (invert && !invert_exp_1 (XEXP (note, 0), jump)))
! 	remove_note (jump, note);
!       else
  	{
! 	  redirect_exp_1 (&XEXP (note, 0), olabel, nlabel, jump);
! 	  apply_change_group ();
  	}
      }
  
    /* If we're eliding the jump over exception cleanups at the end of a
*************** redirect_jump (rtx jump, rtx nlabel, int
*** 1662,1692 ****
    if (olabel && nlabel
        && NEXT_INSN (olabel)
        && NOTE_P (NEXT_INSN (olabel))
!       && NOTE_LINE_NUMBER (NEXT_INSN (olabel)) == NOTE_INSN_FUNCTION_END)
      emit_note_after (NOTE_INSN_FUNCTION_END, nlabel);
  
!   if (olabel && --LABEL_NUSES (olabel) == 0 && delete_unused
        /* Undefined labels will remain outside the insn stream.  */
        && INSN_UID (olabel))
      delete_related_insns (olabel);
- 
-   return 1;
  }
  
! /* Invert the jump condition of rtx X contained in jump insn, INSN.
!    Accrue the modifications into the change group.  */
! 
! static void
! invert_exp_1 (rtx insn)
  {
!   RTX_CODE code;
!   rtx x = pc_set (insn);
! 
!   if (!x)
!     abort ();
!   x = SET_SRC (x);
! 
!   code = GET_CODE (x);
  
    if (code == IF_THEN_ELSE)
      {
--- 1644,1665 ----
    if (olabel && nlabel
        && NEXT_INSN (olabel)
        && NOTE_P (NEXT_INSN (olabel))
!       && NOTE_LINE_NUMBER (NEXT_INSN (olabel)) == NOTE_INSN_FUNCTION_END
!       && delete_unused >= 0)
      emit_note_after (NOTE_INSN_FUNCTION_END, nlabel);
  
!   if (olabel && --LABEL_NUSES (olabel) == 0 && delete_unused > 0
        /* Undefined labels will remain outside the insn stream.  */
        && INSN_UID (olabel))
      delete_related_insns (olabel);
  }
  
! /* Invert the jump condition X contained in jump insn INSN.  Accrue the
!    modifications into the change group.  Return nonzero for success.  */
! static int
! invert_exp_1 (rtx x, rtx insn)
  {
!   RTX_CODE code = GET_CODE (x);
  
    if (code == IF_THEN_ELSE)
      {
*************** invert_exp_1 (rtx insn)
*** 1708,1737 ****
  					   GET_MODE (comp), XEXP (comp, 0),
  					   XEXP (comp, 1)),
  			   1);
! 	  return;
  	}
  
        tem = XEXP (x, 1);
        validate_change (insn, &XEXP (x, 1), XEXP (x, 2), 1);
        validate_change (insn, &XEXP (x, 2), tem, 1);
      }
    else
-     abort ();
- }
- 
- /* Invert the jump condition of conditional jump insn, INSN.
- 
-    Return 1 if we can do so, 0 if we cannot find a way to do so that
-    matches a pattern.  */
- 
- static int
- invert_exp (rtx insn)
- {
-   invert_exp_1 (insn);
-   if (num_validated_changes () == 0)
      return 0;
- 
-   return apply_change_group ();
  }
  
  /* Invert the condition of the jump JUMP, and make it jump to label
--- 1681,1696 ----
  					   GET_MODE (comp), XEXP (comp, 0),
  					   XEXP (comp, 1)),
  			   1);
! 	  return 1;
  	}
  
        tem = XEXP (x, 1);
        validate_change (insn, &XEXP (x, 1), XEXP (x, 2), 1);
        validate_change (insn, &XEXP (x, 2), tem, 1);
+       return 1;
      }
    else
      return 0;
  }
  
  /* Invert the condition of the jump JUMP, and make it jump to label
*************** invert_exp (rtx insn)
*** 1742,1751 ****
  int
  invert_jump_1 (rtx jump, rtx nlabel)
  {
    int ochanges;
  
    ochanges = num_validated_changes ();
!   invert_exp_1 (jump);
    if (num_validated_changes () == ochanges)
      return 0;
  
--- 1701,1712 ----
  int
  invert_jump_1 (rtx jump, rtx nlabel)
  {
+   rtx x = pc_set (jump);
    int ochanges;
  
    ochanges = num_validated_changes ();
!   if (!x || !invert_exp_1 (SET_SRC (x), jump))
!     abort ();
    if (num_validated_changes () == ochanges)
      return 0;
  
*************** invert_jump_1 (rtx jump, rtx nlabel)
*** 1758,1787 ****
  int
  invert_jump (rtx jump, rtx nlabel, int delete_unused)
  {
!   /* We have to either invert the condition and change the label or
!      do neither.  Either operation could fail.  We first try to invert
!      the jump. If that succeeds, we try changing the label.  If that fails,
!      we invert the jump back to what it was.  */
! 
!   if (! invert_exp (jump))
!     return 0;
  
!   if (redirect_jump (jump, nlabel, delete_unused))
      {
!       /* Remove REG_EQUAL note if we have one.  */
!       rtx note = find_reg_note (jump, REG_EQUAL, NULL_RTX);
!       if (note)
! 	remove_note (jump, note);
! 
        invert_br_probabilities (jump);
- 
        return 1;
      }
! 
!   if (! invert_exp (jump))
!     /* This should just be putting it back the way it was.  */
!     abort ();
! 
    return 0;
  }
  
--- 1719,1733 ----
  int
  invert_jump (rtx jump, rtx nlabel, int delete_unused)
  {
!   rtx olabel = JUMP_LABEL (jump);
  
!   if (invert_jump_1 (jump, nlabel) && apply_change_group ())
      {
!       redirect_jump_2 (jump, olabel, nlabel, delete_unused, 1);
        invert_br_probabilities (jump);
        return 1;
      }
!   cancel_changes (0);
    return 0;
  }
  
Index: ifcvt.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/ifcvt.c,v
retrieving revision 1.179
diff -p -r1.179 ifcvt.c
*** ifcvt.c	25 Jan 2005 23:09:07 -0000	1.179
--- ifcvt.c	1 Mar 2005 19:57:58 -0000
*************** dead_or_predicable (basic_block test_bb,
*** 3289,3299 ****
  
    if (other_bb != new_dest)
      {
!       if (old_dest)
! 	LABEL_NUSES (old_dest) -= 1;
!       if (new_label)
! 	LABEL_NUSES (new_label) += 1;
!       JUMP_LABEL (jump) = new_label;
        if (reversep)
  	invert_br_probabilities (jump);
  
--- 3289,3295 ----
  
    if (other_bb != new_dest)
      {
!       redirect_jump_2 (jump, old_dest, new_label, -1, reversep);
        if (reversep)
  	invert_br_probabilities (jump);