diff mbox

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

Message ID 20110404075512.GJ18914@tyan-ft48-01.lab.bos.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek April 4, 2011, 7:55 a.m. UTC 
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.



	Jakub

Comments

Jakub Jelinek April 5, 2011, 8:45 a.m. UTC | #1 
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
Eric Botcazou April 5, 2011, 3:02 p.m. UTC | #2 
> 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.
Alexandre Oliva April 5, 2011, 5:24 p.m. UTC | #3 
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?
Jakub Jelinek April 5, 2011, 5:48 p.m. UTC | #4 
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
Alexandre Oliva April 5, 2011, 11:40 p.m. UTC | #5 
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!
diff mbox

Patch

--- 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;
+    }
+}