[PATCH] Fix VTA updating in the combiner (PR debug/48343)

[PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Jakub Jelinek]

Hi!

We ICE on the attached testcase, because combiner changes mode of
a pseudo (which was set just once and used once plus in debug insns)
from SImode to QImode, but the uses in debug insns aren't adjusted.

Combiner has code to adjust this too (propagate_for_debug), but only updates
debug insns between i2 and i3 (resp. i3 and undobuf.other_insn).
The problem on the testcase is that this is a retry, so first
try_combine with a later i3 calls propagate_for_debug and changes debug
insns before that later i3, then returns an earlier insn that should be
retried and we stop adjusting debug insns at that earlier i3.  Unfortunately
as later debug insns have been already updated earlier, they need to be
adjusted too.

The following patch fixes that by always stopping on the latest i3 that has
been successfully combined into in the current bb, bootstrapped/regtested
on x86_64-linux and i686-linux, ok for trunk/4.6?

2011-03-30  Jakub Jelinek  <jakub@redhat.com>

	PR debug/48343
	* combine.c (last_combined_insn): New variable.
	(combine_instructions): Clear it before processing each bb
	and after last bb.
	(try_combine): Set it to i3 if i3 is after it.
	Use it as the last argument to propagate_for_debug instead
	of i3.
	(distribute_notes): Assert SET_INSN_DELETE is not called
	on last_combined_insn.

	* gcc.dg/torture/pr48343.c: New test.

--- gcc/combine.c.jj	2011-03-23 09:34:40.000000000 +0100
+++ gcc/combine.c	2011-03-30 17:39:09.000000000 +0200
@@ -337,6 +337,9 @@ static enum machine_mode nonzero_bits_mo
 
 static int nonzero_sign_valid;
 
+/* Last insn on which try_combine has been called in the current BB.  */
+
+static rtx last_combined_insn;
 \f
 /* Record one modification to rtl structure
    to be undone by storing old_contents into *where.  */
@@ -1168,6 +1171,7 @@ combine_instructions (rtx f, unsigned in
 	  || single_pred (this_basic_block) != last_bb)
 	label_tick_ebb_start = label_tick;
       last_bb = this_basic_block;
+      last_combined_insn = NULL_RTX;
 
       rtl_profile_for_bb (this_basic_block);
       for (insn = BB_HEAD (this_basic_block);
@@ -1379,6 +1383,7 @@ combine_instructions (rtx f, unsigned in
 	}
     }
 
+  last_combined_insn = NULL_RTX;
   default_rtl_profile ();
   clear_log_links ();
   clear_bb_flags ();
@@ -3830,6 +3835,10 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
       return 0;
     }
 
+  if (last_combined_insn == NULL_RTX
+      || DF_INSN_LUID (last_combined_insn) < DF_INSN_LUID (i3))
+    last_combined_insn = i3;
+
   if (MAY_HAVE_DEBUG_INSNS)
     {
       struct undo *undo;
@@ -3851,7 +3860,7 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 		   i2src while its original mode is temporarily
 		   restored, and then clear i2scratch so that we don't
 		   do it again later.  */
-		propagate_for_debug (i2, i3, reg, i2src);
+		propagate_for_debug (i2, last_combined_insn, reg, i2src);
 		i2scratch = false;
 		/* Put back the new mode.  */
 		adjust_reg_mode (reg, new_mode);
@@ -3859,18 +3868,17 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 	    else
 	      {
 		rtx tempreg = gen_raw_REG (old_mode, REGNO (reg));
-		rtx first, last;
+		rtx first;
 
 		if (reg == i2dest)
-		  {
-		    first = i2;
-		    last = i3;
-		  }
+		  first = i2;
 		else
 		  {
 		    first = i3;
-		    last = undobuf.other_insn;
-		    gcc_assert (last);
+		    gcc_assert (undobuf.other_insn);
+		    if (DF_INSN_LUID (undobuf.other_insn)
+			> DF_INSN_LUID (last_combined_insn))
+		      last_combined_insn = undobuf.other_insn;
 		  }
 
 		/* We're dealing with a reg that changed mode but not
@@ -3882,9 +3890,9 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 		   with this copy we have created; then, replace the
 		   copy with the SUBREG of the original shared reg,
 		   once again changed to the new mode.  */
-		propagate_for_debug (first, last, reg, tempreg);
+		propagate_for_debug (first, last_combined_insn, reg, tempreg);
 		adjust_reg_mode (reg, new_mode);
-		propagate_for_debug (first, last, tempreg,
+		propagate_for_debug (first, last_combined_insn, tempreg,
 				     lowpart_subreg (old_mode, reg, new_mode));
 	      }
 	  }
@@ -4099,14 +4107,14 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
     if (newi2pat)
       {
 	if (MAY_HAVE_DEBUG_INSNS && i2scratch)
-	  propagate_for_debug (i2, i3, i2dest, i2src);
+	  propagate_for_debug (i2, last_combined_insn, i2dest, i2src);
 	INSN_CODE (i2) = i2_code_number;
 	PATTERN (i2) = newi2pat;
       }
     else
       {
 	if (MAY_HAVE_DEBUG_INSNS && i2src)
-	  propagate_for_debug (i2, i3, i2dest, i2src);
+	  propagate_for_debug (i2, last_combined_insn, i2dest, i2src);
 	SET_INSN_DELETED (i2);
       }
 
@@ -4115,7 +4123,7 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 	LOG_LINKS (i1) = 0;
 	REG_NOTES (i1) = 0;
 	if (MAY_HAVE_DEBUG_INSNS)
-	  propagate_for_debug (i1, i3, i1dest, i1src);
+	  propagate_for_debug (i1, last_combined_insn, i1dest, i1src);
 	SET_INSN_DELETED (i1);
       }
 
@@ -4124,7 +4132,7 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 	LOG_LINKS (i0) = 0;
 	REG_NOTES (i0) = 0;
 	if (MAY_HAVE_DEBUG_INSNS)
-	  propagate_for_debug (i0, i3, i0dest, i0src);
+	  propagate_for_debug (i0, last_combined_insn, i0dest, i0src);
 	SET_INSN_DELETED (i0);
       }
 
@@ -13454,6 +13462,7 @@ distribute_notes (rtx notes, rtx from_in
 					    NULL_RTX, NULL_RTX, NULL_RTX);
 			  distribute_links (LOG_LINKS (tem));
 
+			  gcc_assert (tem != last_combined_insn);
 			  SET_INSN_DELETED (tem);
 			  if (tem == i2)
 			    i2 = NULL_RTX;
@@ -13471,6 +13480,7 @@ distribute_notes (rtx notes, rtx from_in
 						NULL_RTX, NULL_RTX, NULL_RTX);
 			      distribute_links (LOG_LINKS (cc0_setter));
 
+			      gcc_assert (cc0_setter != last_combined_insn);
 			      SET_INSN_DELETED (cc0_setter);
 			      if (cc0_setter == i2)
 				i2 = NULL_RTX;
--- gcc/testsuite/gcc.dg/torture/pr48343.c.jj	2011-03-30 17:41:51.000000000 +0200
+++ gcc/testsuite/gcc.dg/torture/pr48343.c	2011-03-30 17:41:32.000000000 +0200
@@ -0,0 +1,19 @@
+/* PR debug/48343 */
+/* { dg-do compile } */
+/* { dg-options "-fcompare-debug" } */
+
+void foo (unsigned char *, unsigned char *);
+
+void
+test (unsigned int x, int y)
+{
+  unsigned int i, j = 0, k;
+  unsigned char s[256], t[64];
+  foo (s, t);
+  t[0] = y;
+  for (i = 0; i < 256; i++)
+    {
+      j = (j + s[i] + t[i % x]) & 0xff;
+      k = i; i = j; j = k;
+    }
+}

	Jakub

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Eric Botcazou]

> Combiner has code to adjust this too (propagate_for_debug), but only
> updates debug insns between i2 and i3 (resp. i3 and undobuf.other_insn).
> The problem on the testcase is that this is a retry, so first
> try_combine with a later i3 calls propagate_for_debug and changes debug
> insns before that later i3, then returns an earlier insn that should be
> retried and we stop adjusting debug insns at that earlier i3. 
> Unfortunately as later debug insns have been already updated earlier, they
> need to be adjusted too.
>
> The following patch fixes that by always stopping on the latest i3 that has
> been successfully combined into in the current bb, bootstrapped/regtested
> on x86_64-linux and i686-linux, ok for trunk/4.6?

This seems to be a rare problem though so I'm not sure we should directly go 
for an "always" approach.  IIUC this can happen only when try_combine returns 
a NEXT to combine_instructions which is before INSN in the stream, right?
Can't we precisely detect this case under the "retry:" label and pass an 
additional argument to try_combine?  Do we really need to adjust all the calls 
to propagate_for_debug and not just the ones made for UNDO_MODE when the 
register is I2DEST?

-- 
Eric Botcazou

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Jakub Jelinek]

On Sat, Apr 02, 2011 at 05:42:55PM +0200, Eric Botcazou wrote:
> > Combiner has code to adjust this too (propagate_for_debug), but only
> > updates debug insns between i2 and i3 (resp. i3 and undobuf.other_insn).
> > The problem on the testcase is that this is a retry, so first
> > try_combine with a later i3 calls propagate_for_debug and changes debug
> > insns before that later i3, then returns an earlier insn that should be
> > retried and we stop adjusting debug insns at that earlier i3. 
> > Unfortunately as later debug insns have been already updated earlier, they
> > need to be adjusted too.
> >
> > The following patch fixes that by always stopping on the latest i3 that has
> > been successfully combined into in the current bb, bootstrapped/regtested
> > on x86_64-linux and i686-linux, ok for trunk/4.6?
> 
> This seems to be a rare problem though so I'm not sure we should directly go 
> for an "always" approach.  IIUC this can happen only when try_combine returns 
> a NEXT to combine_instructions which is before INSN in the stream, right?

Yeah, but that is when in the patch i3 will differ from last_combined_insn.

> Can't we precisely detect this case under the "retry:" label and pass an 
> additional argument to try_combine?  Do we really need to adjust all the calls 

We'd still need to do the LUID checking anyway, to find out when we still do
retry earlier insns and when we return back to looking at new insns.
Plus, I believe whenever we propagate_for_debug after i3 (the updating in
between i3 and undobuf.other_insn, we need to adjust those insns too.

> to propagate_for_debug and not just the ones made for UNDO_MODE when the 
> register is I2DEST?

I think we need to update there in all cases.  The reason we don't need to
update beyond i3 resp. undobuf.other_insn is that DF guarantees us that
there won't be debug insns referring to those pseudos afterwards, otherwise
either the pseudo must be live afterwards in real code (then it wouldn't
be a single use case), or debug insns would be reset, or a debug temporary
would be created, where the temporary is set before last place where
the pseudo is used in real code.  Now, once we propagate_for_debug after
some insn, DF hasn't been run in between and thus the pseudos might be live
afterwards.

If you just want to avoid a global variable, the code can be surely changed
to have a local variable from combine_instructions and pass address to that
to all try_combine calls, but other than that I think we should do what the
patch does.
Alex, do you agree?

	Jakub

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Eric Botcazou]

> I think we need to update there in all cases.  The reason we don't need to
> update beyond i3 resp. undobuf.other_insn is that DF guarantees us that
> there won't be debug insns referring to those pseudos afterwards, otherwise
> either the pseudo must be live afterwards in real code (then it wouldn't
> be a single use case), or debug insns would be reset, or a debug temporary
> would be created, where the temporary is set before last place where
> the pseudo is used in real code.  Now, once we propagate_for_debug after
> some insn, DF hasn't been run in between and thus the pseudos might be live
> afterwards.

Frankly moving down last_combined_insn to undobuf.other_insn in the UNDO_MODE 
case seems a little overengineered at this point.

> If you just want to avoid a global variable, the code can be surely changed
> to have a local variable from combine_instructions and pass address to that
> to all try_combine calls, but other than that I think we should do what the
> patch does.

I'd eliminate the global variable and directly pass the insn to try_combine, 
this is good enough for now IMO.

-- 
Eric Botcazou

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Jakub Jelinek]

On Sun, Apr 03, 2011 at 11:39:22AM +0200, Eric Botcazou wrote:
> > I think we need to update there in all cases.  The reason we don't need to
> > update beyond i3 resp. undobuf.other_insn is that DF guarantees us that
> > there won't be debug insns referring to those pseudos afterwards, otherwise
> > either the pseudo must be live afterwards in real code (then it wouldn't
> > be a single use case), or debug insns would be reset, or a debug temporary
> > would be created, where the temporary is set before last place where
> > the pseudo is used in real code.  Now, once we propagate_for_debug after
> > some insn, DF hasn't been run in between and thus the pseudos might be live
> > afterwards.
> 
> Frankly moving down last_combined_insn to undobuf.other_insn in the UNDO_MODE 
> case seems a little overengineered at this point.

You are right, in that case we only replace regs with lowpart subreg
thereof, so it shouldn't extend the lifetime of any pseudo.

> > If you just want to avoid a global variable, the code can be surely changed
> > to have a local variable from combine_instructions and pass address to that
> > to all try_combine calls, but other than that I think we should do what the
> > patch does.
> 
> I'd eliminate the global variable and directly pass the insn to try_combine, 
> this is good enough for now IMO.

So something like this?

Alternatively, perhaps all we care about is either i3, or NEXT_INSN of the last
debug_insn propagate_for_debug changed in an interesting way.  Thus
propagate_for_debug could return the last DEBUG_INSN it changed, and caller
decide either that no updating of last_modified_debug_insn is needed (that
is the case of the pair of propagate_for_debug calls from first to last
which just make it a lowpart subreg), or it would update it.

2011-04-04  Jakub Jelinek  <jakub@redhat.com>

	PR debug/48343
	* combine.c (combine_instructions): Add last_combined_insn,
	update it if insn is after it, pass it to all try_combine
	calls.
	(try_combine): Add last_combined_insn parameter, pass it instead of
	i3 to propagate_for_debug.

	* gcc.dg/torture/pr48343.c: New test.

--- gcc/combine.c.jj	2011-04-04 08:56:08.000000000 +0200
+++ gcc/combine.c	2011-04-04 09:41:13.000000000 +0200
@@ -387,7 +387,7 @@ static int cant_combine_insn_p (rtx);
 static int can_combine_p (rtx, rtx, rtx, rtx, rtx, rtx, rtx *, rtx *);
 static int combinable_i3pat (rtx, rtx *, rtx, rtx, rtx, int, int, rtx *);
 static int contains_muldiv (rtx);
-static rtx try_combine (rtx, rtx, rtx, rtx, int *);
+static rtx try_combine (rtx, rtx, rtx, rtx, int *, rtx);
 static void undo_all (void);
 static void undo_commit (void);
 static rtx *find_split_point (rtx *, rtx, bool);
@@ -1159,6 +1159,7 @@ combine_instructions (rtx f, unsigned in
 
   FOR_EACH_BB (this_basic_block)
     {
+      rtx last_combined_insn = NULL_RTX;
       optimize_this_for_speed_p = optimize_bb_for_speed_p (this_basic_block);
       last_call_luid = 0;
       mem_last_set = -1;
@@ -1177,6 +1178,10 @@ combine_instructions (rtx f, unsigned in
 	  next = 0;
 	  if (NONDEBUG_INSN_P (insn))
 	    {
+	      if (last_combined_insn == NULL_RTX
+		  || DF_INSN_LUID (last_combined_insn) < DF_INSN_LUID (insn))
+		last_combined_insn = insn;
+
 	      /* See if we know about function return values before this
 		 insn based upon SUBREG flags.  */
 	      check_promoted_subreg (insn, PATTERN (insn));
@@ -1190,7 +1195,8 @@ combine_instructions (rtx f, unsigned in
 
 	      for (links = LOG_LINKS (insn); links; links = XEXP (links, 1))
 		if ((next = try_combine (insn, XEXP (links, 0), NULL_RTX,
-					 NULL_RTX, &new_direct_jump_p)) != 0)
+					 NULL_RTX, &new_direct_jump_p,
+					 last_combined_insn)) != 0)
 		  goto retry;
 
 	      /* Try each sequence of three linked insns ending with this one.  */
@@ -1208,8 +1214,8 @@ combine_instructions (rtx f, unsigned in
 		       nextlinks;
 		       nextlinks = XEXP (nextlinks, 1))
 		    if ((next = try_combine (insn, link, XEXP (nextlinks, 0),
-					     NULL_RTX,
-					     &new_direct_jump_p)) != 0)
+					     NULL_RTX, &new_direct_jump_p,
+					     last_combined_insn)) != 0)
 		      goto retry;
 		}
 
@@ -1227,14 +1233,15 @@ combine_instructions (rtx f, unsigned in
 		  && sets_cc0_p (PATTERN (prev)))
 		{
 		  if ((next = try_combine (insn, prev, NULL_RTX, NULL_RTX,
-					   &new_direct_jump_p)) != 0)
+					   &new_direct_jump_p,
+					   last_combined_insn)) != 0)
 		    goto retry;
 
 		  for (nextlinks = LOG_LINKS (prev); nextlinks;
 		       nextlinks = XEXP (nextlinks, 1))
 		    if ((next = try_combine (insn, prev, XEXP (nextlinks, 0),
-					     NULL_RTX,
-					     &new_direct_jump_p)) != 0)
+					     NULL_RTX, &new_direct_jump_p,
+					     last_combined_insn)) != 0)
 		      goto retry;
 		}
 
@@ -1247,14 +1254,15 @@ combine_instructions (rtx f, unsigned in
 		  && reg_mentioned_p (cc0_rtx, SET_SRC (PATTERN (insn))))
 		{
 		  if ((next = try_combine (insn, prev, NULL_RTX, NULL_RTX,
-					   &new_direct_jump_p)) != 0)
+					   &new_direct_jump_p,
+					   last_combined_insn)) != 0)
 		    goto retry;
 
 		  for (nextlinks = LOG_LINKS (prev); nextlinks;
 		       nextlinks = XEXP (nextlinks, 1))
 		    if ((next = try_combine (insn, prev, XEXP (nextlinks, 0),
-					     NULL_RTX,
-					     &new_direct_jump_p)) != 0)
+					     NULL_RTX, &new_direct_jump_p,
+					     last_combined_insn)) != 0)
 		      goto retry;
 		}
 
@@ -1269,8 +1277,8 @@ combine_instructions (rtx f, unsigned in
 		    && NONJUMP_INSN_P (prev)
 		    && sets_cc0_p (PATTERN (prev))
 		    && (next = try_combine (insn, XEXP (links, 0),
-					    prev, NULL_RTX,
-					    &new_direct_jump_p)) != 0)
+					    prev, NULL_RTX, &new_direct_jump_p,
+					    last_combined_insn)) != 0)
 		  goto retry;
 #endif
 
@@ -1281,7 +1289,8 @@ combine_instructions (rtx f, unsigned in
 		     nextlinks = XEXP (nextlinks, 1))
 		  if ((next = try_combine (insn, XEXP (links, 0),
 					   XEXP (nextlinks, 0), NULL_RTX,
-					   &new_direct_jump_p)) != 0)
+					   &new_direct_jump_p,
+					   last_combined_insn)) != 0)
 		    goto retry;
 
 	      /* Try four-instruction combinations.  */
@@ -1305,14 +1314,16 @@ combine_instructions (rtx f, unsigned in
 			   nextlinks = XEXP (nextlinks, 1))
 			if ((next = try_combine (insn, link, link1,
 						 XEXP (nextlinks, 0),
-						 &new_direct_jump_p)) != 0)
+						 &new_direct_jump_p,
+						 last_combined_insn)) != 0)
 			  goto retry;
 		      /* I0, I1 -> I2, I2 -> I3.  */
 		      for (nextlinks = XEXP (next1, 1); nextlinks;
 			   nextlinks = XEXP (nextlinks, 1))
 			if ((next = try_combine (insn, link, link1,
 						 XEXP (nextlinks, 0),
-						 &new_direct_jump_p)) != 0)
+						 &new_direct_jump_p,
+						 last_combined_insn)) != 0)
 			  goto retry;
 		    }
 
@@ -1326,14 +1337,16 @@ combine_instructions (rtx f, unsigned in
 			   nextlinks = XEXP (nextlinks, 1))
 			if ((next = try_combine (insn, link, link1,
 						 XEXP (nextlinks, 0),
-						 &new_direct_jump_p)) != 0)
+						 &new_direct_jump_p,
+						 last_combined_insn)) != 0)
 			  goto retry;
 		      /* I0 -> I1; I1, I2 -> I3.  */
 		      for (nextlinks = LOG_LINKS (link1); nextlinks;
 			   nextlinks = XEXP (nextlinks, 1))
 			if ((next = try_combine (insn, link, link1,
 						 XEXP (nextlinks, 0),
-						 &new_direct_jump_p)) != 0)
+						 &new_direct_jump_p,
+						 last_combined_insn)) != 0)
 			  goto retry;
 		    }
 		}
@@ -1362,7 +1375,8 @@ combine_instructions (rtx f, unsigned in
 		      i2mod_old_rhs = copy_rtx (orig);
 		      i2mod_new_rhs = copy_rtx (note);
 		      next = try_combine (insn, i2mod, NULL_RTX, NULL_RTX,
-					  &new_direct_jump_p);
+					  &new_direct_jump_p,
+					  last_combined_insn);
 		      i2mod = NULL_RTX;
 		      if (next)
 			goto retry;
@@ -2501,10 +2515,15 @@ update_cfg_for_uncondjump (rtx insn)
    resume scanning.
 
    Set NEW_DIRECT_JUMP_P to a nonzero value if try_combine creates a
-   new direct jump instruction.  */
+   new direct jump instruction.
+
+   LAST_COMBINED_INSN is either I3, or some insn after I3 that has
+   been I3 passed to an earlier try_combine within the same basic
+   block.  */
 
 static rtx
-try_combine (rtx i3, rtx i2, rtx i1, rtx i0, int *new_direct_jump_p)
+try_combine (rtx i3, rtx i2, rtx i1, rtx i0, int *new_direct_jump_p,
+	     rtx last_combined_insn)
 {
   /* New patterns for I3 and I2, respectively.  */
   rtx newpat, newi2pat = 0;
@@ -3851,7 +3870,7 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 		   i2src while its original mode is temporarily
 		   restored, and then clear i2scratch so that we don't
 		   do it again later.  */
-		propagate_for_debug (i2, i3, reg, i2src);
+		propagate_for_debug (i2, last_combined_insn, reg, i2src);
 		i2scratch = false;
 		/* Put back the new mode.  */
 		adjust_reg_mode (reg, new_mode);
@@ -3864,13 +3883,16 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 		if (reg == i2dest)
 		  {
 		    first = i2;
-		    last = i3;
+		    last = last_combined_insn;
 		  }
 		else
 		  {
 		    first = i3;
 		    last = undobuf.other_insn;
 		    gcc_assert (last);
+		    if (DF_INSN_LUID (last)
+			< DF_INSN_LUID (last_combined_insn))
+		      last = last_combined_insn;
 		  }
 
 		/* We're dealing with a reg that changed mode but not
@@ -4098,14 +4120,14 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
     if (newi2pat)
       {
 	if (MAY_HAVE_DEBUG_INSNS && i2scratch)
-	  propagate_for_debug (i2, i3, i2dest, i2src);
+	  propagate_for_debug (i2, last_combined_insn, i2dest, i2src);
 	INSN_CODE (i2) = i2_code_number;
 	PATTERN (i2) = newi2pat;
       }
     else
       {
 	if (MAY_HAVE_DEBUG_INSNS && i2src)
-	  propagate_for_debug (i2, i3, i2dest, i2src);
+	  propagate_for_debug (i2, last_combined_insn, i2dest, i2src);
 	SET_INSN_DELETED (i2);
       }
 
@@ -4114,7 +4136,7 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 	LOG_LINKS (i1) = 0;
 	REG_NOTES (i1) = 0;
 	if (MAY_HAVE_DEBUG_INSNS)
-	  propagate_for_debug (i1, i3, i1dest, i1src);
+	  propagate_for_debug (i1, last_combined_insn, i1dest, i1src);
 	SET_INSN_DELETED (i1);
       }
 
@@ -4123,7 +4145,7 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
 	LOG_LINKS (i0) = 0;
 	REG_NOTES (i0) = 0;
 	if (MAY_HAVE_DEBUG_INSNS)
-	  propagate_for_debug (i0, i3, i0dest, i0src);
+	  propagate_for_debug (i0, last_combined_insn, i0dest, i0src);
 	SET_INSN_DELETED (i0);
       }
 
--- gcc/testsuite/gcc.dg/torture/pr48343.c.jj	2011-04-04 09:31:33.000000000 +0200
+++ gcc/testsuite/gcc.dg/torture/pr48343.c	2011-04-04 09:31:33.000000000 +0200
@@ -0,0 +1,19 @@
+/* PR debug/48343 */
+/* { dg-do compile } */
+/* { dg-options "-fcompare-debug" } */
+
+void foo (unsigned char *, unsigned char *);
+
+void
+test (unsigned int x, int y)
+{
+  unsigned int i, j = 0, k;
+  unsigned char s[256], t[64];
+  foo (s, t);
+  t[0] = y;
+  for (i = 0; i < 256; i++)
+    {
+      j = (j + s[i] + t[i % x]) & 0xff;
+      k = i; i = j; j = k;
+    }
+}


	Jakub

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Jakub Jelinek]

On Mon, Apr 04, 2011 at 09:55:12AM +0200, Jakub Jelinek wrote:
> So something like this?

Now bootstrapped/regtested on x86_64-linux and i686-linux, ok?

> 2011-04-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/48343
> 	* combine.c (combine_instructions): Add last_combined_insn,
> 	update it if insn is after it, pass it to all try_combine
> 	calls.
> 	(try_combine): Add last_combined_insn parameter, pass it instead of
> 	i3 to propagate_for_debug.
> 
> 	* gcc.dg/torture/pr48343.c: New test.

	Jakub

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Eric Botcazou]

> Alternatively, perhaps all we care about is either i3, or NEXT_INSN of the
> last debug_insn propagate_for_debug changed in an interesting way.  Thus
> propagate_for_debug could return the last DEBUG_INSN it changed, and caller
> decide either that no updating of last_modified_debug_insn is needed (that
> is the case of the pair of propagate_for_debug calls from first to last
> which just make it a lowpart subreg), or it would update it.

Maybe.  I was more or less waiting for Alexandre's take on all this. :-)

> 2011-04-04  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR debug/48343
> 	* combine.c (combine_instructions): Add last_combined_insn,
> 	update it if insn is after it, pass it to all try_combine
> 	calls.
> 	(try_combine): Add last_combined_insn parameter, pass it instead of
> 	i3 to propagate_for_debug.
>
> 	* gcc.dg/torture/pr48343.c: New test.

This version is fine by me at least, thanks.

-- 
Eric Botcazou

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Alexandre Oliva]

On Apr  4, 2011, Jakub Jelinek <jakub@redhat.com> wrote:

> Alternatively, perhaps all we care about is either i3, or NEXT_INSN of
> the last debug_insn propagate_for_debug changed in an interesting way.

Ideally, we'd like to identify all debug users of i2dest (or whatever
DEF we're removing) and propagate into them, even if they are e.g. in
different blocks or after i3.

IIRC stopping at i3 is a trade-off that avoids making the analysis more
complex, just because we know i2dest, if removed, isn't used after i3
any more, although it might still be used in debug insns.  I don't
recall how/when(/whether) we adjust those afterwards.

Anyhow, I don't quite understand what makes the retry case special.
Each try_combine call (and its debug propagations) are supposed to be
self-contained.  If we dropped a SET, we should adjust all debug uses of
its SET_DEST; if we didn't drop it, no adjustment is needed.  It seems
to me that extending the range might even cause us to modify
e.g. unrelated uses of the same pseudo, no?  Isn't the problem perhaps
that we are failing to adjust some debug insns at an earlier call?

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Jakub Jelinek]

On Tue, Apr 05, 2011 at 02:24:55PM -0300, Alexandre Oliva wrote:
> On Apr  4, 2011, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> > Alternatively, perhaps all we care about is either i3, or NEXT_INSN of
> > the last debug_insn propagate_for_debug changed in an interesting way.
> 
> Ideally, we'd like to identify all debug users of i2dest (or whatever
> DEF we're removing) and propagate into them, even if they are e.g. in
> different blocks or after i3.

If they are in different blocks, then either there must be some other
use of the pseudo in real code after that (but then combine won't do such
replacements), or DF would have resetted those debug_insns referencing it
(or replaced the pseudos in there with a debug temporary set before the last
real use), otherwise debug insns would change the liveness of the pseudo.

> IIRC stopping at i3 is a trade-off that avoids making the analysis more
> complex, just because we know i2dest, if removed, isn't used after i3
> any more, although it might still be used in debug insns.  I don't
> recall how/when(/whether) we adjust those afterwards.

The change mode of pseudos propagation can't be a best effort, we really
must change all the pseudos (or reset debug_insns containing them),
otherwise the RTL in the insns might be invalid.
The other changes we propagate are best effort, if we don't change all,
and some pseudo remains referenced only in debug insns and not really set
in real code or used anywhere, next DF recomputation will just reset them.
Of course the less often that happens, the better.

> Anyhow, I don't quite understand what makes the retry case special.
> Each try_combine call (and its debug propagations) are supposed to be
> self-contained.  If we dropped a SET, we should adjust all debug uses of
> its SET_DEST; if we didn't drop it, no adjustment is needed.  It seems
> to me that extending the range might even cause us to modify
> e.g. unrelated uses of the same pseudo, no?  Isn't the problem perhaps
> that we are failing to adjust some debug insns at an earlier call?

The reason why retry case is special is that without any previous
propagate_for_debug calls we know that e.g. in the change mode case,
the pseudo can't be referenced before i2 and after i3 where i2 is
the single setter of the pseudo and i3 is the single (non-debug) user.
Otherwise RTL would not be valid before combiner.  But with some previous
propagate_for_debug calls in the same bb that guarantee (as the testcase
shows) is no longer true.  We might have propagate_for_debug, replacing
some i2dest with i2src in between i2 and i3, then goto retry with new
i3' being somewhere before previous i3.  We change mode of the pseudo
that was present in old i2src, but if we stop looking for debug insns
at i3', there might be some debug insn in between i3' and i3 that
references that pseudo.

	Jakub

Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Alexandre Oliva]

On Apr  5, 2011, Jakub Jelinek <jakub@redhat.com> wrote:

> or DF would have resetted those debug_insns referencing it
> (or replaced the pseudos in there with a debug temporary set before the last
> real use), otherwise debug insns would change the liveness of the pseudo.

Ah, right, that was the bit I was missing!  (it's unfortunate that SSA
and DF are so different in this regard)

> But with some previous propagate_for_debug calls in the same bb that
> guarantee (as the testcase shows) is no longer true.

I see.  The patch looks like the right fix, then.  Thanks!

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer