Re: PATCH for the mainline COMMITTED.

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

Ian Lance Taylor wrote:
> Kenneth Zadeck <zadeck@naturalbridge.com> writes:
>
>   
> > This patch was developed by Ian from a bug that Danny and I hit when we were moving the web pass after combine.  The bug appears to be a silent failure when web is either not run or run before combine.
> >
> > Combine incorrectly modifies the mode of a register if the moon and
> > several planets are in the correct alignment. The patch was tested two
> > ways.  The bug disappears on ppc-suse-linux and
> > x86_64-unknown-linux-gnu when using a lot of experimental code.
> >     
>
> To clarify a bit, there is code in combine to change the mode of a
> pseudo-register, when profitable, if the register is set only once;
> see can_change_dest_mode.  However the test in can_change_dest_mode
> does not consider that the pseudo-register might appear in REG_EQUAL
> notes.  There is an attempt to fix up that case in distribute_notes,
> but it doesn't catch all possibilities.  Most obviously it doesn't
> catch the case where the insn with the note is not combined with
> anything, and hence the notes are never passed to distribute_notes.
>
> The bug can be seen by looking at debugging dumps.  After combine the
> same pseudo-register appears with one mode in the insn and with
> another mode in a REG_EQUAL note.
>
> We keep a canonical version of each pseudo-register in the
> regno_reg_rtx array, so it is easy to update every use of the
> pseudo-register by updating that array.  The old code used to simply
> replace the entry in the array, which of course did not affect other
> existing uses.  This patch changes it to directly modify the entry in
> the array, by slightly extending the existing combine undo mechanism.
>
> Since I wrote this patch, it looks good to me.  I'll approve it, but
> please wait 48 hours to commit to see if there are any comments.
>
>   

2006-01-11  Ian Lance Taylor <ian@airs.com>

     * combine.c (struct undo): Remove is_int.  Enumify types
     of undos.  Allow undoing set of machine mode.
     (do_SUBST): Use enums instead of is_int.
     (do_SUBST_MODE): New function.
     (SUBST_MODE): New macro.
     (try_combine): Use SUBST_MODE/PUT_MODE instead of generating
     a new reg and trying to replace reg_regno_rtx with a new
register.
     (undo_all): Use new enums, handle undoing a PUT_MODE change.
     (simplify_set): Use SUBST_MODE.
     (distribute_notes): Remove code that tried to update reg notes
for regno_reg_rtx changes.

> > 2006-01-09  Ian Taylors <ian@airs.com>
> >     
>
> ChangeLog entry should be "Ian Lance Taylor"--see ChangeLog-2005 for
> examples.
>
> Thanks for testing it.
>
> Ian
>   

Index: testsuite/gcc.c-torture/compile/20060109-1.c
===================================================================
--- testsuite/gcc.c-torture/compile/20060109-1.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/20060109-1.c	(revision 0)
@@ -0,0 +1,36 @@
+/* This test exposed a bug in combine where it was improperly changing
+   the mode of a register.  The bug appeared to be latent until web
+   was moved after combine.  This is the reduced test that fails 
+   by crashing in reload.  */
+
+
+typedef struct cpp_reader cpp_reader;
+typedef struct cpp_string cpp_string;
+struct cpp_string
+{
+  unsigned int len;
+  const unsigned char *text;
+};
+struct cpp_callbacks
+{
+  void (*ident) (cpp_reader *, unsigned int, const cpp_string *);
+};
+static void cb_ident (cpp_reader *, unsigned int, const cpp_string *);
+init_c_lex (void)
+{
+  struct cpp_callbacks *cb;
+  cb->ident = cb_ident;
+}
+cb_ident (cpp_reader * pfile __attribute__ ((__unused__)), unsigned int
+line
+          __attribute__ ((__unused__)), const cpp_string * str
+          __attribute__ ((__unused__)))
+{
+  {
+    cpp_string cstr = {
+    };
+    if (cpp_interpret_string (pfile, str, 1, &cstr, 0))
+      {
+      }
+  }
+}
Index: combine.c
===================================================================
--- combine.c	(revision 109577)
+++ combine.c	(working copy)
@@ -321,15 +321,14 @@ static int nonzero_sign_valid;
 
 
 /* Record one modification to rtl structure
-   to be undone by storing old_contents into *where.
-   is_int is 1 if the contents are an int.  */
+   to be undone by storing old_contents into *where.  */
 
 struct undo
 {
   struct undo *next;
-  int is_int;
-  union {rtx r; int i;} old_contents;
-  union {rtx *r; int *i;} where;
+  enum { UNDO_RTX, UNDO_INT, UNDO_MODE } kind;
+  union { rtx r; int i; enum machine_mode m; } old_contents;
+  union { rtx *r; int *i; } where;
 };
 
 /* Record a bunch of changes to be undone, up to MAX_UNDO of them.
@@ -490,7 +489,7 @@ do_SUBST (rtx *into, rtx newval)
   else
     buf = xmalloc (sizeof (struct undo));
 
-  buf->is_int = 0;
+  buf->kind = UNDO_RTX;
   buf->where.r = into;
   buf->old_contents.r = oldval;
   *into = newval;
@@ -518,7 +517,7 @@ do_SUBST_INT (int *into, int newval)
   else
     buf = xmalloc (sizeof (struct undo));
 
-  buf->is_int = 1;
+  buf->kind = UNDO_INT;
   buf->where.i = into;
   buf->old_contents.i = oldval;
   *into = newval;
@@ -527,6 +526,35 @@ do_SUBST_INT (int *into, int newval)
 }
 
 #define SUBST_INT(INTO, NEWVAL)  do_SUBST_INT(&(INTO), (NEWVAL))
+
+/* Similar to SUBST, but just substitute the mode.  This is used when
+   changing the mode of a pseudo-register, so that any other
+   references to the entry in the regno_reg_rtx array will change as
+   well.  */
+
+static void
+do_SUBST_MODE (rtx *into, enum machine_mode newval)
+{
+  struct undo *buf;
+  enum machine_mode oldval = GET_MODE (*into);
+
+  if (oldval == newval)
+    return;
+
+  if (undobuf.frees)
+    buf = undobuf.frees, undobuf.frees = buf->next;
+  else
+    buf = xmalloc (sizeof (struct undo));
+
+  buf->kind = UNDO_MODE;
+  buf->where.r = into;
+  buf->old_contents.m = oldval;
+  PUT_MODE (*into, newval);
+
+  buf->next = undobuf.undos, undobuf.undos = buf;
+}
+
+#define SUBST_MODE(INTO, NEWVAL)  do_SUBST_MODE(&(INTO), (NEWVAL))
 
 /* Subroutine of try_combine.  Determine whether the combine replacement
    patterns NEWPAT and NEWI2PAT are cheaper according to insn_rtx_cost
@@ -2224,10 +2252,15 @@ try_combine (rtx i3, rtx i2, rtx i1, int
 				   compare_mode))
 	    {
 	      unsigned int regno = REGNO (SET_DEST (newpat));
-	      rtx new_dest = gen_rtx_REG (compare_mode, regno);
+	      rtx new_dest;
 
-	      if (regno >= FIRST_PSEUDO_REGISTER)
-		SUBST (regno_reg_rtx[regno], new_dest);
+	      if (regno < FIRST_PSEUDO_REGISTER)
+		new_dest = gen_rtx_REG (compare_mode, regno);
+	      else
+		{
+		  SUBST_MODE (regno_reg_rtx[regno], compare_mode);
+		  new_dest = regno_reg_rtx[regno];
+		}
 
 	      SUBST (SET_DEST (newpat), new_dest);
 	      SUBST (XEXP (*cc_use, 0), new_dest);
@@ -2476,7 +2509,6 @@ try_combine (rtx i3, rtx i2, rtx i1, int
       && asm_noperands (newpat) < 0)
     {
       rtx m_split, *split;
-      rtx ni2dest = i2dest;
 
       /* See if the MD file can split NEWPAT.  If it can't, see if letting it
 	 use I2DEST as a scratch register will help.  In the latter case,
@@ -2491,34 +2523,55 @@ try_combine (rtx i3, rtx i2, rtx i1, int
 	 possible to try that as a scratch reg.  This would require adding
 	 more code to make it work though.  */
 
-      if (m_split == 0 && ! reg_overlap_mentioned_p (ni2dest, newpat))
+      if (m_split == 0 && ! reg_overlap_mentioned_p (i2dest, newpat))
 	{
- 	  enum machine_mode new_mode = GET_MODE (SET_DEST (newpat));
-	  /* If I2DEST is a hard register or the only use of a pseudo,
-	     we can change its mode.  */
- 	  if (new_mode != GET_MODE (i2dest)
- 	      && new_mode != VOIDmode
- 	      && can_change_dest_mode (i2dest, added_sets_2, new_mode))
-	    ni2dest = gen_rtx_REG (GET_MODE (SET_DEST (newpat)),
-				   REGNO (i2dest));
+	  enum machine_mode new_mode = GET_MODE (SET_DEST (newpat));
 
+	  /* First try to split using the original register as a
+	     scratch register.  */
 	  m_split = split_insns (gen_rtx_PARALLEL
 				 (VOIDmode,
 				  gen_rtvec (2, newpat,
 					     gen_rtx_CLOBBER (VOIDmode,
-							      ni2dest))),
+							      i2dest))),
 				 i3);
-	  /* If the split with the mode-changed register didn't work, try
-	     the original register.  */
-	  if (! m_split && ni2dest != i2dest)
+
+	  /* If that didn't work, try changing the mode of I2DEST if
+	     we can.  */
+	  if (m_split == 0
+	      && new_mode != GET_MODE (i2dest)
+	      && new_mode != VOIDmode
+	      && can_change_dest_mode (i2dest, added_sets_2, new_mode))
 	    {
-	      ni2dest = i2dest;
+	      enum machine_mode old_mode = GET_MODE (i2dest);
+	      rtx ni2dest;
+
+	      if (REGNO (i2dest) < FIRST_PSEUDO_REGISTER)
+		ni2dest = gen_rtx_REG (new_mode, REGNO (i2dest));
+	      else
+		{
+		  SUBST_MODE (regno_reg_rtx[REGNO (i2dest)], new_mode);
+		  ni2dest = regno_reg_rtx[REGNO (i2dest)];
+		}
+
 	      m_split = split_insns (gen_rtx_PARALLEL
 				     (VOIDmode,
 				      gen_rtvec (2, newpat,
 						 gen_rtx_CLOBBER (VOIDmode,
-								  i2dest))),
+								  ni2dest))),
 				     i3);
+
+	      if (m_split == 0
+		  && REGNO (i2dest) >= FIRST_PSEUDO_REGISTER)
+		{
+		  struct undo *buf;
+
+		  PUT_MODE (regno_reg_rtx[REGNO (i2dest)], old_mode);
+		  buf = undobuf.undos;
+		  undobuf.undos = buf->next;
+		  buf->next = undobuf.frees;
+		  undobuf.frees = buf;
+		}
 	    }
 	}
 
@@ -2547,13 +2600,6 @@ try_combine (rtx i3, rtx i2, rtx i1, int
 	  i3set = single_set (NEXT_INSN (m_split));
 	  i2set = single_set (m_split);
 
-	  /* In case we changed the mode of I2DEST, replace it in the
-	     pseudo-register table here.  We can't do it above in case this
-	     code doesn't get executed and we do a split the other way.  */
-
-	  if (REGNO (i2dest) >= FIRST_PSEUDO_REGISTER)
-	    SUBST (regno_reg_rtx[REGNO (i2dest)], ni2dest);
-
 	  i2_code_number = recog_for_combine (&newi2pat, i2, &new_i2_notes);
 
 	  /* If I2 or I3 has multiple SETs, we won't know how to track
@@ -2624,10 +2670,13 @@ try_combine (rtx i3, rtx i2, rtx i1, int
 	     validated that we can do this.  */
 	  if (GET_MODE (i2dest) != split_mode && split_mode != VOIDmode)
 	    {
-	      newdest = gen_rtx_REG (split_mode, REGNO (i2dest));
-
-	      if (REGNO (i2dest) >= FIRST_PSEUDO_REGISTER)
-		SUBST (regno_reg_rtx[REGNO (i2dest)], newdest);
+	      if (REGNO (i2dest) < FIRST_PSEUDO_REGISTER)
+		newdest = gen_rtx_REG (split_mode, REGNO (i2dest));
+	      else
+		{
+		  SUBST_MODE (regno_reg_rtx[REGNO (i2dest)], split_mode);
+		  newdest = regno_reg_rtx[REGNO (i2dest)];
+		}
 	    }
 
 	  /* If *SPLIT is a (mult FOO (const_int pow2)), convert it to
@@ -3328,10 +3377,20 @@ undo_all (void)
   for (undo = undobuf.undos; undo; undo = next)
     {
       next = undo->next;
-      if (undo->is_int)
-	*undo->where.i = undo->old_contents.i;
-      else
-	*undo->where.r = undo->old_contents.r;
+      switch (undo->kind)
+	{
+	case UNDO_RTX:
+	  *undo->where.r = undo->old_contents.r;
+	  break;
+	case UNDO_INT:
+	  *undo->where.i = undo->old_contents.i;
+	  break;
+	case UNDO_MODE:
+	  PUT_MODE (*undo->where.r, undo->old_contents.m);
+	  break;
+	default:
+	  gcc_unreachable ();
+	}
 
       undo->next = undobuf.frees;
       undobuf.frees = undo;
@@ -5160,10 +5219,15 @@ simplify_set (rtx x)
 	  if (can_change_dest_mode (dest, 0, compare_mode))
 	    {
 	      unsigned int regno = REGNO (dest);
-	      rtx new_dest = gen_rtx_REG (compare_mode, regno);
+	      rtx new_dest;
 
-	      if (regno >= FIRST_PSEUDO_REGISTER)
-		SUBST (regno_reg_rtx[regno], new_dest);
+	      if (regno < FIRST_PSEUDO_REGISTER)
+		new_dest = gen_rtx_REG (compare_mode, regno);
+	      else
+		{
+		  SUBST_MODE (regno_reg_rtx[regno], compare_mode);
+		  new_dest = regno_reg_rtx[regno];
+		}
 
 	      SUBST (SET_DEST (x), new_dest);
 	      SUBST (XEXP (*cc_use, 0), new_dest);
@@ -11709,12 +11773,6 @@ distribute_notes (rtx notes, rtx from_in
     {
       rtx place = 0, place2 = 0;
 
-      /* If this NOTE references a pseudo register, ensure it references
-	 the latest copy of that register.  */
-      if (XEXP (note, 0) && REG_P (XEXP (note, 0))
-	  && REGNO (XEXP (note, 0)) >= FIRST_PSEUDO_REGISTER)
-	XEXP (note, 0) = regno_reg_rtx[REGNO (XEXP (note, 0))];
-
       next_note = XEXP (note, 1);
       switch (REG_NOTE_KIND (note))
 	{