Patchwork sched-deps.c deps->last_pending_memory_flush fix (PR rtl-optimization/46614)

login
register
mail settings
Submitter Jakub Jelinek
Date Nov. 23, 2010, 5:22 p.m.
Message ID <20101123172211.GT29412@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/72704/
State New
Headers show

Comments

Jakub Jelinek - Nov. 23, 2010, 5:22 p.m.
Hi!

As mentioned in the PR, the testcase is miscompiled, because both
sched_analyze_insn and sched_analyze_2 assume that all JUMP_INSNs in
the deps->last_pending_memory_flush list were added there in
           if (deps->pending_flush_length++ > MAX_PENDING_LIST_LENGTH)
             flush_pending_lists (deps, insn, true, true);    
           else
	     deps->last_pending_memory_flush
	       = alloc_INSN_LIST (insn, deps->last_pending_memory_flush); <==== here
and thus handles them as jumps.  But, a JUMP_INSN may be added there
also through flush_pending_lists, be it in the above listed call
or in another spot, and at that point the JUMP_INSN doesn't stand
solely for itself, but also for all memory stores etc. on which
that insn depends.  And such a JUMP_INSN should be handled like
all non-JUMP_INSN insns on the flush last_pending_memory_flush
list.

Fixed by instead remembering if a JUMP_INSN stands only for itself
and nothing else in the deps->last_pending_memory_flush list
(such jumps have REG_NOTE_KIND set to REG_DEP_ANTI, could be
anything but REG_DEP_TRUE which is the default and used for
additions to the list from flush_pending_lists) and using REG_NOTE_KIND
instead of JUMP_P checks in the two places which care about it.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2010-11-23  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/46614
	* sched-deps.c (deps_analyze_insn): Mark JUMP_INSNs in
	last_pending_memory_flush that weren't added through
	flush_pending_lists with REG_DEP_ANTI.
	(sched_analyze_2, sched_analyze_insn): Check REG_NOTE_KIND
	on INSN_LIST instead of JUMP_P check on its operand.
	* sched-rgn.c (concat_INSN_LIST): Copy over REG_NOTE_KIND.

	* gcc.dg/pr46614.c: New test.


	Jakub
Vladimir Makarov - Nov. 23, 2010, 6:54 p.m.
On 11/23/2010 12:22 PM, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, the testcase is miscompiled, because both
> sched_analyze_insn and sched_analyze_2 assume that all JUMP_INSNs in
> the deps->last_pending_memory_flush list were added there in
>             if (deps->pending_flush_length++>  MAX_PENDING_LIST_LENGTH)
>               flush_pending_lists (deps, insn, true, true);
>             else
> 	     deps->last_pending_memory_flush
> 	       = alloc_INSN_LIST (insn, deps->last_pending_memory_flush);<==== here
> and thus handles them as jumps.  But, a JUMP_INSN may be added there
> also through flush_pending_lists, be it in the above listed call
> or in another spot, and at that point the JUMP_INSN doesn't stand
> solely for itself, but also for all memory stores etc. on which
> that insn depends.  And such a JUMP_INSN should be handled like
> all non-JUMP_INSN insns on the flush last_pending_memory_flush
> list.
>
> Fixed by instead remembering if a JUMP_INSN stands only for itself
> and nothing else in the deps->last_pending_memory_flush list
> (such jumps have REG_NOTE_KIND set to REG_DEP_ANTI, could be
> anything but REG_DEP_TRUE which is the default and used for
> additions to the list from flush_pending_lists) and using REG_NOTE_KIND
> instead of JUMP_P checks in the two places which care about it.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
It looks ok for me, Jakub.  Although it would be nice to add a comment 
about meaning REG_DEP_ANTI for element of list last_pending_memory_flush 
when you set it up in function deps_analyze_insn.

Thanks.
> 2010-11-23  Jakub Jelinek<jakub@redhat.com>
>
> 	PR rtl-optimization/46614
> 	* sched-deps.c (deps_analyze_insn): Mark JUMP_INSNs in
> 	last_pending_memory_flush that weren't added through
> 	flush_pending_lists with REG_DEP_ANTI.
> 	(sched_analyze_2, sched_analyze_insn): Check REG_NOTE_KIND
> 	on INSN_LIST instead of JUMP_P check on its operand.
> 	* sched-rgn.c (concat_INSN_LIST): Copy over REG_NOTE_KIND.
>
> 	* gcc.dg/pr46614.c: New test.
>
> --- gcc/sched-deps.c.jj	2010-11-01 09:07:23.000000000 +0100
> +++ gcc/sched-deps.c	2010-11-23 15:59:44.000000000 +0100
> @@ -2484,7 +2484,7 @@ sched_analyze_2 (struct deps_desc *deps,
>
>   	    for (u = deps->last_pending_memory_flush; u; u = XEXP (u, 1))
>   	      {
> -		if (! JUMP_P (XEXP (u, 0)))
> +		if (REG_NOTE_KIND (u) != REG_DEP_ANTI)
>   		  add_dependence (insn, XEXP (u, 0), REG_DEP_ANTI);
>   		else if (deps_may_trap_p (x))
>   		  {
> @@ -2796,7 +2796,7 @@ sched_analyze_insn (struct deps_desc *de
>   			   REG_DEP_ANTI);
>
>         for (u = deps->last_pending_memory_flush; u; u = XEXP (u, 1))
> -	if (! JUMP_P (XEXP (u, 0))
> +	if (REG_NOTE_KIND (u) != REG_DEP_ANTI
>   	    || !sel_sched_p ())
>   	  add_dependence (insn, XEXP (u, 0), REG_DEP_ANTI);
>
> @@ -3242,8 +3242,12 @@ deps_analyze_insn (struct deps_desc *dep
>             if (deps->pending_flush_length++>  MAX_PENDING_LIST_LENGTH)
>               flush_pending_lists (deps, insn, true, true);
>             else
> -            deps->last_pending_memory_flush
> -              = alloc_INSN_LIST (insn, deps->last_pending_memory_flush);
> +	    {
> +	      deps->last_pending_memory_flush
> +		= alloc_INSN_LIST (insn, deps->last_pending_memory_flush);
> +	      PUT_REG_NOTE_KIND (deps->last_pending_memory_flush,
> +				 REG_DEP_ANTI);
> +	    }
>           }
>
>         sched_analyze_insn (deps, PATTERN (insn), insn);
> --- gcc/sched-rgn.c.jj	2010-11-01 09:07:24.000000000 +0100
> +++ gcc/sched-rgn.c	2010-11-23 16:00:29.000000000 +0100
> @@ -2574,7 +2574,10 @@ concat_INSN_LIST (rtx copy, rtx old)
>   {
>     rtx new_rtx = old;
>     for (; copy ; copy = XEXP (copy, 1))
> -    new_rtx = alloc_INSN_LIST (XEXP (copy, 0), new_rtx);
> +    {
> +      new_rtx = alloc_INSN_LIST (XEXP (copy, 0), new_rtx);
> +      PUT_REG_NOTE_KIND (new_rtx, REG_NOTE_KIND (copy));
> +    }
>     return new_rtx;
>   }
>
> --- gcc/testsuite/gcc.dg/pr46614.c.jj	2010-11-23 16:06:07.000000000 +0100
> +++ gcc/testsuite/gcc.dg/pr46614.c	2010-11-23 16:05:44.000000000 +0100
> @@ -0,0 +1,56 @@
> +/* PR rtl-optimization/46614 */
> +/* { dg-do run } */
> +/* { dg-options "-O -fno-rename-registers -fsched2-use-superblocks -fschedule-insns2 -funroll-loops" } */
> +
> +extern void abort (void);
> +
> +struct S
> +{
> +  unsigned char a;
> +  unsigned char b;
> +  unsigned int c;
> +  unsigned int e;
> +  unsigned char f;
> +  unsigned int g;
> +};
> +
> +void bar (struct S *x)
> +{
> +  int i;
> +  struct S *p = x;
> +  struct S r[16];
> +  unsigned j;
> +  for (i = 0; i<  16; i++)
> +    {
> +      r[i].c = p->b + p->c;
> +      j = p->c + p->f;
> +      r[i].a = j + p->b;
> +      r[i].f = p->f + p->e;
> +      r[i].g = p->b + p->c;
> +    }
> +  for (i = 0; i<  16; i++)
> +    {
> +      if (r[i].c != x[i].b + x[i].c
> +	  || r[i].a != x[i].c + x[i].f + x[i].b
> +	  || r[i].f != x[i].f + x[i].e
> +	  || r[i].g != x[i].b + x[i].c)
> +	abort ();
> +    }
> +  for (i = 0; i<  16; i++)
> +    {
> +      r[i].b = p->c;
> +      if (r[i].b != x[i].c)
> +	abort ();
> +    }
> +}
> +
> +int
> +main ()
> +{
> +  int i;
> +  struct S x[16];
> +  for (i = 0; i<  16; i++)
> +    x[i].b = x[i].c = x[i].e = x[i].f = 5;
> +  bar (x);
> +  return 0;
> +}
>
> 	Jakub
Jakub Jelinek - Nov. 23, 2010, 10:39 p.m.
On Tue, Nov 23, 2010 at 01:54:41PM -0500, Vladimir Makarov wrote:
>  On 11/23/2010 12:22 PM, Jakub Jelinek wrote:
> >Fixed by instead remembering if a JUMP_INSN stands only for itself
> >and nothing else in the deps->last_pending_memory_flush list
> >(such jumps have REG_NOTE_KIND set to REG_DEP_ANTI, could be
> >anything but REG_DEP_TRUE which is the default and used for
> >additions to the list from flush_pending_lists) and using REG_NOTE_KIND
> >instead of JUMP_P checks in the two places which care about it.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> It looks ok for me, Jakub.  Although it would be nice to add a
> comment about meaning REG_DEP_ANTI for element of list
> last_pending_memory_flush when you set it up in function
> deps_analyze_insn.

Maybe also define macros?

/* In deps->last_pending_memory_flush marks JUMP_INSNs that weren't
   added to the list because of flush_pending_lists, stands just
   for itself and not for any other pending memory reads/writes.  */
#define NON_FLUSH_JUMP_KIND REG_DEP_ANTI
#define NON_FLUSH_JUMP_P(x) (REG_NOTE_KIND (x) == REG_DEP_ANTI)

	Jakub

Patch

--- gcc/sched-deps.c.jj	2010-11-01 09:07:23.000000000 +0100
+++ gcc/sched-deps.c	2010-11-23 15:59:44.000000000 +0100
@@ -2484,7 +2484,7 @@  sched_analyze_2 (struct deps_desc *deps,
 
 	    for (u = deps->last_pending_memory_flush; u; u = XEXP (u, 1))
 	      {
-		if (! JUMP_P (XEXP (u, 0)))
+		if (REG_NOTE_KIND (u) != REG_DEP_ANTI)
 		  add_dependence (insn, XEXP (u, 0), REG_DEP_ANTI);
 		else if (deps_may_trap_p (x))
 		  {
@@ -2796,7 +2796,7 @@  sched_analyze_insn (struct deps_desc *de
 			   REG_DEP_ANTI);
 
       for (u = deps->last_pending_memory_flush; u; u = XEXP (u, 1))
-	if (! JUMP_P (XEXP (u, 0))
+	if (REG_NOTE_KIND (u) != REG_DEP_ANTI
 	    || !sel_sched_p ())
 	  add_dependence (insn, XEXP (u, 0), REG_DEP_ANTI);
 
@@ -3242,8 +3242,12 @@  deps_analyze_insn (struct deps_desc *dep
           if (deps->pending_flush_length++ > MAX_PENDING_LIST_LENGTH)
             flush_pending_lists (deps, insn, true, true);
           else
-            deps->last_pending_memory_flush
-              = alloc_INSN_LIST (insn, deps->last_pending_memory_flush);
+	    {
+	      deps->last_pending_memory_flush
+		= alloc_INSN_LIST (insn, deps->last_pending_memory_flush);
+	      PUT_REG_NOTE_KIND (deps->last_pending_memory_flush,
+				 REG_DEP_ANTI);
+	    }
         }
 
       sched_analyze_insn (deps, PATTERN (insn), insn);
--- gcc/sched-rgn.c.jj	2010-11-01 09:07:24.000000000 +0100
+++ gcc/sched-rgn.c	2010-11-23 16:00:29.000000000 +0100
@@ -2574,7 +2574,10 @@  concat_INSN_LIST (rtx copy, rtx old)
 {
   rtx new_rtx = old;
   for (; copy ; copy = XEXP (copy, 1))
-    new_rtx = alloc_INSN_LIST (XEXP (copy, 0), new_rtx);
+    {
+      new_rtx = alloc_INSN_LIST (XEXP (copy, 0), new_rtx);
+      PUT_REG_NOTE_KIND (new_rtx, REG_NOTE_KIND (copy));
+    }
   return new_rtx;
 }
 
--- gcc/testsuite/gcc.dg/pr46614.c.jj	2010-11-23 16:06:07.000000000 +0100
+++ gcc/testsuite/gcc.dg/pr46614.c	2010-11-23 16:05:44.000000000 +0100
@@ -0,0 +1,56 @@ 
+/* PR rtl-optimization/46614 */
+/* { dg-do run } */
+/* { dg-options "-O -fno-rename-registers -fsched2-use-superblocks -fschedule-insns2 -funroll-loops" } */
+
+extern void abort (void);
+
+struct S
+{
+  unsigned char a;
+  unsigned char b;
+  unsigned int c;
+  unsigned int e;
+  unsigned char f;
+  unsigned int g;
+};
+
+void bar (struct S *x)
+{
+  int i;
+  struct S *p = x;
+  struct S r[16];
+  unsigned j;
+  for (i = 0; i < 16; i++)
+    {
+      r[i].c = p->b + p->c;
+      j = p->c + p->f;
+      r[i].a = j + p->b;
+      r[i].f = p->f + p->e;
+      r[i].g = p->b + p->c;
+    }
+  for (i = 0; i < 16; i++)
+    {
+      if (r[i].c != x[i].b + x[i].c
+	  || r[i].a != x[i].c + x[i].f + x[i].b
+	  || r[i].f != x[i].f + x[i].e
+	  || r[i].g != x[i].b + x[i].c)
+	abort ();
+    }
+  for (i = 0; i < 16; i++)
+    {
+      r[i].b = p->c;
+      if (r[i].b != x[i].c)
+	abort ();
+    }
+}
+
+int
+main ()
+{
+  int i;
+  struct S x[16];
+  for (i = 0; i < 16; i++)
+    x[i].b = x[i].c = x[i].e = x[i].f = 5;
+  bar (x);
+  return 0;
+}