Patchwork Fix combiner ICEs after my recent patch (PR rtl-optimization/48549)

login
register
mail settings
Submitter Jakub Jelinek
Date April 12, 2011, 8:06 a.m.
Message ID <20110412080647.GC17079@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/90738/
State New
Headers show

Comments

Jakub Jelinek - April 12, 2011, 8:06 a.m.
On Tue, Apr 12, 2011 at 09:18:01AM +0200, Eric Botcazou wrote:
> > With the second patch, bootstrap succeeded even with gcc_assert (at_end);
> > in update_cfg_for_uncondjump, so either it is always at the end, or at
> > least very often, thus perhaps with the second patch being applied
> > in combine_instructions we could just for INSN_DELETED_P clear
> > last_combined_insn right away, meaning we want to propagate till end of
> > current bb.
> 
> Something I don't understand at the moment: why moving last_combined_insn down 
> to the next insn instead of up to the previous one when it has been deleted?

That is because propagate_for_debug stops at the insn before last, doesn't
process last any longer.  So, if there is a DEBUG_INSN right before the jump
being deleted, and it has been propagated into, after the jump is deleted
and retry is done at i2 earlier, following propagate_for_debug won't see
that DEBUG_INSN any longer.

We could certainly change propagate_for_debug, so that it would stop either
before end (NEXT_INSN (BB_END (this_basic_block)) added in this patch), or
after processing last (which doesn't change anything in the non-deleted insn
case, because i3 from any try_combine surely isn't a DEBUG_INSN and
propagate_for_debug only propagates into those), then on deletion set it to
PREV_INSN instead of NEXT_INSN.  I wonder whether we couldn't be deleting
the first insn in a basic block, but probably there should be at least a
note before it.

> Wouldn't the latter simplify things, in particular avoid setting it to NULL_RTX 
> which looks rather awkward to me?

Below is an untested patch (well, just tested on the testcase in question).
If that's what you prefer, I'll bootstrap/regtest it.

Perhaps instead of the assert we could in that case just set
last_combined_insn to insn.

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

	PR rtl-optimization/48549
	* combine.c (propagate_for_debug): Also stop after BB_END of
	this_basic_block.  Process LAST and just stop processing after it.
	(combine_instructions): If last_combined_insn has been deleted,
	set last_combined_insn to its PREV_INSN.

	* g++.dg/opt/pr48549.C: New test.



	Jakub
Eric Botcazou - April 12, 2011, 8:29 a.m.
> That is because propagate_for_debug stops at the insn before last, doesn't
> process last any longer.  So, if there is a DEBUG_INSN right before the
> jump being deleted, and it has been propagated into, after the jump is
> deleted and retry is done at i2 earlier, following propagate_for_debug
> won't see that DEBUG_INSN any longer.

Ah, yes, thanks.

> We could certainly change propagate_for_debug, so that it would stop either
> before end (NEXT_INSN (BB_END (this_basic_block)) added in this patch), or
> after processing last (which doesn't change anything in the non-deleted
> insn case, because i3 from any try_combine surely isn't a DEBUG_INSN and
> propagate_for_debug only propagates into those), then on deletion set it to
> PREV_INSN instead of NEXT_INSN.  I wonder whether we couldn't be deleting
> the first insn in a basic block, but probably there should be at least a
> note before it.

This looks fine to me.

> Perhaps instead of the assert we could in that case just set
> last_combined_insn to insn.

Yes, this looks preferable to me, IMO we don't need to stop the compiler when 
something goes wrong with debug insns.

> 2011-04-12  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR rtl-optimization/48549
> 	* combine.c (propagate_for_debug): Also stop after BB_END of
> 	this_basic_block.  Process LAST and just stop processing after it.
> 	(combine_instructions): If last_combined_insn has been deleted,
> 	set last_combined_insn to its PREV_INSN.
>
> 	* g++.dg/opt/pr48549.C: New test.

So OK modulo the last point, thanks.

Patch

--- gcc/combine.c.jj	2011-04-12 09:37:36.000000000 +0200
+++ gcc/combine.c	2011-04-12 09:50:44.000000000 +0200
@@ -1198,8 +1198,18 @@  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))
+	      if (last_combined_insn)
+		{
+		  while (INSN_DELETED_P (last_combined_insn))
+		    last_combined_insn = PREV_INSN (last_combined_insn);
+		  gcc_assert (!BARRIER_P (last_combined_insn)
+			      && BLOCK_FOR_INSN (last_combined_insn)
+				 == this_basic_block);
+		  if (DF_INSN_LUID (last_combined_insn)
+		      <= DF_INSN_LUID (insn))
+		    last_combined_insn = insn;
+		}
+	      else
 		last_combined_insn = insn;
 
 	      /* See if we know about function return values before this
@@ -2446,19 +2456,21 @@  propagate_for_debug_subst (rtx from, con
 }
 
 /* Replace all the occurrences of DEST with SRC in DEBUG_INSNs between INSN
-   and LAST.  */
+   and LAST, not including INSN, but including LAST.  Also stop at the end
+   of THIS_BASIC_BLOCK.  */
 
 static void
 propagate_for_debug (rtx insn, rtx last, rtx dest, rtx src)
 {
-  rtx next, loc;
+  rtx next, loc, end = NEXT_INSN (BB_END (this_basic_block));
 
   struct rtx_subst_pair p;
   p.to = src;
   p.adjusted = false;
 
   next = NEXT_INSN (insn);
-  while (next != last)
+  last = NEXT_INSN (last);
+  while (next != last && next != end)
     {
       insn = next;
       next = NEXT_INSN (insn);
--- gcc/testsuite/g++.dg/opt/pr48549.C.jj	2011-04-12 09:41:52.000000000 +0200
+++ gcc/testsuite/g++.dg/opt/pr48549.C	2011-04-12 09:41:52.000000000 +0200
@@ -0,0 +1,63 @@ 
+// PR rtl-optimization/48549
+// { dg-do compile }
+// { dg-options "-fcompare-debug -O2" }
+
+void
+foo (void *from, void *to)
+{
+  long offset = reinterpret_cast <long>(to) - reinterpret_cast <long>(from);
+  if (offset != static_cast <int>(offset))
+    *(int *) 0xC0DE = 0;
+  reinterpret_cast <int *>(from)[1] = offset;
+}
+struct A
+{
+  A () : a () {}
+  A (void *x) : a (x) {}
+  void *bar () { return a; }
+  void *a;
+};
+struct C;
+struct D;
+struct E : public A
+{
+  C m1 (int);
+  D m2 ();
+  E () {}
+  E (A x) : A (x) {}
+};
+struct C : public E
+{
+  C () {}
+  C (void *x) : E (x) {}
+};
+struct D : public E
+{
+  D (void *x) : E (x) {}
+};
+C
+E::m1 (int x)
+{
+  return (reinterpret_cast <char *>(bar ()) + x);
+}
+D
+E::m2 ()
+{
+  return reinterpret_cast <char *>(bar ());
+}
+struct B
+{
+  E a;
+  unsigned b : 16;
+  unsigned c : 1;
+};
+void
+baz (B *x)
+{
+  for (unsigned i = 0; i < 64; i++)
+    {
+      D d = x[i].a.m2 ();
+      C c = x[i].a.m1 (x[i].c);
+      foo (d.bar (), c.bar ());
+    }
+}