Patchwork Fix PR 53701

login
register
mail settings
Submitter Andrey Belevantsev
Date Aug. 9, 2012, 11:13 a.m.
Message ID <50239B68.1010208@ispras.ru>
Download mbox | patch
Permalink /patch/176054/
State New
Headers show

Comments

Andrey Belevantsev - Aug. 9, 2012, 11:13 a.m.
Hello,

The problem in question is uncovered by the recent speculation patch, it is 
in the handling of expressions blocked by bookkeeping.  Those are 
expressions that become unavailable due to the newly created bookkeeping 
copies.  In the original algorithm the supported insns and transformations 
cannot lead to this result, but when handling non-separable insns or 
creating speculative checks that unpredictably block certain insns the 
situation can arise.  We just filter out all such expressions from the 
final availability set for correctness.

The PR happens because the expression being filtered out can be transformed 
while being moved up, thus we need to look up not only its exact pattern 
but also all its previous forms saved in its history of changes.  The patch 
does exactly that, I also clarified the comments w.r.t. this situation.

Bootstrapped and tested on ia64 and x86-64, the PR testcase is minimized, 
too.  OK for trunk?  Also need to backport this to 4.7 with PR 53975, say 
on the next week.

Yours,
Andrey

gcc:
2012-08-09  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/53701
	* sel-sched.c (vinsn_vec_has_expr_p): Clarify function comment.
	Process not only expr's vinsns but all old vinsns from expr's
	history of changes.
	(update_and_record_unavailable_insns): Clarify comment.

testsuite:
2012-08-09  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/53701
	* gcc.dg/pr53701.c: New test.
Alexander Monakov - Aug. 9, 2012, 1:19 p.m.
On Thu, 9 Aug 2012, Andrey Belevantsev wrote:

> Hello,
> 
> The problem in question is uncovered by the recent speculation patch, it is in
> the handling of expressions blocked by bookkeeping.  Those are expressions
> that become unavailable due to the newly created bookkeeping copies.  In the
> original algorithm the supported insns and transformations cannot lead to this
> result, but when handling non-separable insns or creating speculative checks
> that unpredictably block certain insns the situation can arise.  We just
> filter out all such expressions from the final availability set for
> correctness.
> 
> The PR happens because the expression being filtered out can be transformed
> while being moved up, thus we need to look up not only its exact pattern but
> also all its previous forms saved in its history of changes.  The patch does
> exactly that, I also clarified the comments w.r.t. this situation.
> 
> Bootstrapped and tested on ia64 and x86-64, the PR testcase is minimized, too.
> OK for trunk?  Also need to backport this to 4.7 with PR 53975, say on the
> next week.

This is OK.

Thanks.

Alexander
Andrey Belevantsev - Oct. 16, 2012, 7:50 a.m.
Hello,

On 09.08.2012 17:19, Alexander Monakov wrote:
>
>
> On Thu, 9 Aug 2012, Andrey Belevantsev wrote:
>
>> Hello,
>>
>> The problem in question is uncovered by the recent speculation patch, it is in
>> the handling of expressions blocked by bookkeeping.  Those are expressions
>> that become unavailable due to the newly created bookkeeping copies.  In the
>> original algorithm the supported insns and transformations cannot lead to this
>> result, but when handling non-separable insns or creating speculative checks
>> that unpredictably block certain insns the situation can arise.  We just
>> filter out all such expressions from the final availability set for
>> correctness.
>>
>> The PR happens because the expression being filtered out can be transformed
>> while being moved up, thus we need to look up not only its exact pattern but
>> also all its previous forms saved in its history of changes.  The patch does
>> exactly that, I also clarified the comments w.r.t. this situation.
>>
>> Bootstrapped and tested on ia64 and x86-64, the PR testcase is minimized, too.
>> OK for trunk?  Also need to backport this to 4.7 with PR 53975, say on the
>> next week.
>
> This is OK.

The below is the port of this patch to 4.7, took longer than expected but 
still.  Will commit after retesting on x86-64 (testing on ia64 is already 
fine) and with the fix for PR 53975.

Andrey

2012-10-16  Andrey Belevantsev  <abel@ispras.ru>

	Backport from mainline
	2012-08-09  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/53701
	* sel-sched.c (vinsn_vec_has_expr_p): Clarify function comment.
	Process not only expr's vinsns but all old vinsns from expr's
	history of changes.
	(update_and_record_unavailable_insns): Clarify comment.	

testsuite:
2012-10-16  Andrey Belevantsev  <abel@ispras.ru>

	Backport from mainline
	2012-08-09  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/53701
	* gcc.dg/pr53701.c: New test.


Index: gcc/testsuite/gcc.dg/pr53701.c
===================================================================
*** gcc/testsuite/gcc.dg/pr53701.c      (revision 0)
--- gcc/testsuite/gcc.dg/pr53701.c      (revision 0)
***************
*** 0 ****
--- 1,59 ----
+ /* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+ /* { dg-options "-O3 -fselective-scheduling2 -fsel-sched-pipelining" } */
+ typedef unsigned short int uint16_t;
+ typedef unsigned long int uintptr_t;
+ typedef struct GFX_VTABLE
+ {
+   int color_depth;
+   unsigned char *line[];
+ }
+ BITMAP;
+ extern int _drawing_mode;
+ extern BITMAP *_drawing_pattern;
+ extern int _drawing_y_anchor;
+ extern unsigned int _drawing_x_mask;
+ extern unsigned int _drawing_y_mask;
+ extern uintptr_t bmp_write_line (BITMAP *, int);
+   void
+ _linear_hline15 (BITMAP * dst, int dx1, int dy, int dx2, int color)
+ {
+   int w;
+   if (_drawing_mode == 0)
+   {
+     int x, curw;
+     unsigned short *sline =
+       (unsigned short *) (_drawing_pattern->
+           line[((dy) -
+             _drawing_y_anchor) & _drawing_y_mask]);
+     unsigned short *s;
+     unsigned short *d =
+       ((unsigned short *) (bmp_write_line (dst, dy)) + (dx1));
+     s = ((unsigned short *) (sline) + (x));
+     if (_drawing_mode == 2)
+     {
+     }
+     else if (_drawing_mode == 3)
+     {
+       do
+       {
+         w -= curw;
+         do
+         {
+           unsigned long c = (*(s));
+           if (!((unsigned long) (c) == 0x7C1F))
+           {
+             (*((uint16_t *) ((uintptr_t) (d))) = ((color)));
+           }
+           ((s)++);
+         }
+         while (--curw > 0);
+         s = sline;
+         curw =
+           (((w) <
+             ((int) _drawing_x_mask +
+              1)) ? (w) : ((int) _drawing_x_mask + 1));
+       }
+       while (curw > 0);
+     }
+   }
+ }
Index: gcc/sel-sched.c
===================================================================
*** gcc/sel-sched.c     (revision 192488)
--- gcc/sel-sched.c     (working copy)
*************** process_use_exprs (av_set_t *av_ptr)
*** 3567,3595 ****
     return NULL;
   }

! /* Lookup EXPR in VINSN_VEC and return TRUE if found.  */
   static bool
   vinsn_vec_has_expr_p (vinsn_vec_t vinsn_vec, expr_t expr)
   {
!   vinsn_t vinsn;
     int n;

!   FOR_EACH_VEC_ELT (vinsn_t, vinsn_vec, n, vinsn)
!     if (VINSN_SEPARABLE_P (vinsn))
!       {
!         if (vinsn_equal_p (vinsn, EXPR_VINSN (expr)))
!           return true;
!       }
!     else
!       {
!         /* For non-separable instructions, the blocking insn can have
!            another pattern due to substitution, and we can't choose
!            different register as in the above case.  Check all registers
!            being written instead.  */
!         if (bitmap_intersect_p (VINSN_REG_SETS (vinsn),
!                                 VINSN_REG_SETS (EXPR_VINSN (expr))))
!           return true;
!       }

     return false;
   }
--- 3567,3607 ----
     return NULL;
   }

! /* Lookup EXPR in VINSN_VEC and return TRUE if found.  Also check 
patterns from
!    EXPR's history of changes.  */
   static bool
   vinsn_vec_has_expr_p (vinsn_vec_t vinsn_vec, expr_t expr)
   {
!   vinsn_t vinsn, expr_vinsn;
     int n;
+   unsigned i;

!   /* Start with checking expr itself and then proceed with all the old forms
!      of expr taken from its history vector.  */
!   for (i = 0, expr_vinsn = EXPR_VINSN (expr);
!        expr_vinsn;
!        expr_vinsn = (i < VEC_length (expr_history_def,
!                                    EXPR_HISTORY_OF_CHANGES (expr))
!                    ? VEC_index (expr_history_def,
!                                 EXPR_HISTORY_OF_CHANGES (expr),
!                                 i++)->old_expr_vinsn
!                    : NULL))
!     FOR_EACH_VEC_ELT (vinsn_t, vinsn_vec, n, vinsn)
!       if (VINSN_SEPARABLE_P (vinsn))
!       {
!         if (vinsn_equal_p (vinsn, expr_vinsn))
!           return true;
!       }
!       else
!       {
!         /* For non-separable instructions, the blocking insn can have
!            another pattern due to substitution, and we can't choose
!            different register as in the above case.  Check all registers
!            being written instead.  */
!         if (bitmap_intersect_p (VINSN_REG_SETS (vinsn),
!                                 VINSN_REG_SETS (expr_vinsn)))
!           return true;
!       }

     return false;
   }
*************** update_and_record_unavailable_insns (bas
*** 5697,5704 ****
                 || EXPR_TARGET_AVAILABLE (new_expr)
                  != EXPR_TARGET_AVAILABLE (cur_expr))
             /* Unfortunately, the below code could be also fired up on
!              separable insns.
!              FIXME: add an example of how this could happen.  */
               vinsn_vec_add (&vec_bookkeeping_blocked_vinsns, cur_expr);
           }

--- 5709,5716 ----
                 || EXPR_TARGET_AVAILABLE (new_expr)
                  != EXPR_TARGET_AVAILABLE (cur_expr))
             /* Unfortunately, the below code could be also fired up on
!              separable insns, e.g. when moving insns through the new
!              speculation check as in PR 53701.  */
               vinsn_vec_add (&vec_bookkeeping_blocked_vinsns, cur_expr);
           }
Andrey Belevantsev - Oct. 22, 2012, 7:41 p.m.
On 16.10.2012 11:50, Andrey Belevantsev wrote:
>
> The below is the port of this patch to 4.7, took longer than expected but
> still.  Will commit after retesting on x86-64 (testing on ia64 is already
> fine) and with the fix for PR 53975.

Now the same patch is also committed to 4.6 after more wait and testing.

Andrey

Patch

diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index 3099b92..f0c6eaf 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -3564,29 +3564,41 @@  process_use_exprs (av_set_t *av_ptr)
   return NULL;
 }
 
-/* Lookup EXPR in VINSN_VEC and return TRUE if found.  */
+/* Lookup EXPR in VINSN_VEC and return TRUE if found.  Also check patterns from
+   EXPR's history of changes.  */
 static bool
 vinsn_vec_has_expr_p (vinsn_vec_t vinsn_vec, expr_t expr)
 {
-  vinsn_t vinsn;
+  vinsn_t vinsn, expr_vinsn;
   int n;
+  unsigned i;
 
-  FOR_EACH_VEC_ELT (vinsn_t, vinsn_vec, n, vinsn)
-    if (VINSN_SEPARABLE_P (vinsn))
-      {
-        if (vinsn_equal_p (vinsn, EXPR_VINSN (expr)))
-          return true;
-      }
-    else
-      {
-        /* For non-separable instructions, the blocking insn can have
-           another pattern due to substitution, and we can't choose
-           different register as in the above case.  Check all registers
-           being written instead.  */
-        if (bitmap_intersect_p (VINSN_REG_SETS (vinsn),
-                                VINSN_REG_SETS (EXPR_VINSN (expr))))
-          return true;
-      }
+  /* Start with checking expr itself and then proceed with all the old forms
+     of expr taken from its history vector.  */
+  for (i = 0, expr_vinsn = EXPR_VINSN (expr);
+       expr_vinsn;
+       expr_vinsn = (i < VEC_length (expr_history_def,
+				     EXPR_HISTORY_OF_CHANGES (expr))
+		     ? VEC_index (expr_history_def,
+				  EXPR_HISTORY_OF_CHANGES (expr),
+				  i++)->old_expr_vinsn
+		     : NULL))
+    FOR_EACH_VEC_ELT (vinsn_t, vinsn_vec, n, vinsn)
+      if (VINSN_SEPARABLE_P (vinsn))
+	{
+	  if (vinsn_equal_p (vinsn, expr_vinsn))
+	    return true;
+	}
+      else
+	{
+	  /* For non-separable instructions, the blocking insn can have
+	     another pattern due to substitution, and we can't choose
+	     different register as in the above case.  Check all registers
+	     being written instead.  */
+	  if (bitmap_intersect_p (VINSN_REG_SETS (vinsn),
+				  VINSN_REG_SETS (expr_vinsn)))
+	    return true;
+	}
 
   return false;
 }
@@ -5694,8 +5706,8 @@  update_and_record_unavailable_insns (basic_block book_block)
               || EXPR_TARGET_AVAILABLE (new_expr)
 		 != EXPR_TARGET_AVAILABLE (cur_expr))
 	    /* Unfortunately, the below code could be also fired up on
-	       separable insns.
-	       FIXME: add an example of how this could happen.  */
+	       separable insns, e.g. when moving insns through the new
+	       speculation check as in PR 53701.  */
             vinsn_vec_add (&vec_bookkeeping_blocked_vinsns, cur_expr);
         }
 
diff --git a/gcc/testsuite/gcc.dg/pr53701.c b/gcc/testsuite/gcc.dg/pr53701.c
new file mode 100644
index 0000000..2c85223
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr53701.c
@@ -0,0 +1,59 @@ 
+/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O3 -fselective-scheduling2 -fsel-sched-pipelining" } */
+typedef unsigned short int uint16_t;
+typedef unsigned long int uintptr_t;
+typedef struct GFX_VTABLE
+{
+  int color_depth;
+  unsigned char *line[];
+}
+BITMAP;
+extern int _drawing_mode;
+extern BITMAP *_drawing_pattern;
+extern int _drawing_y_anchor;
+extern unsigned int _drawing_x_mask;
+extern unsigned int _drawing_y_mask;
+extern uintptr_t bmp_write_line (BITMAP *, int);
+  void
+_linear_hline15 (BITMAP * dst, int dx1, int dy, int dx2, int color)
+{
+  int w;
+  if (_drawing_mode == 0)
+  {
+    int x, curw;
+    unsigned short *sline =
+      (unsigned short *) (_drawing_pattern->
+          line[((dy) -
+            _drawing_y_anchor) & _drawing_y_mask]);
+    unsigned short *s;
+    unsigned short *d =
+      ((unsigned short *) (bmp_write_line (dst, dy)) + (dx1));
+    s = ((unsigned short *) (sline) + (x));
+    if (_drawing_mode == 2)
+    {
+    }
+    else if (_drawing_mode == 3)
+    {
+      do
+      {
+        w -= curw;
+        do
+        {
+          unsigned long c = (*(s));
+          if (!((unsigned long) (c) == 0x7C1F))
+          {
+            (*((uint16_t *) ((uintptr_t) (d))) = ((color)));
+          }
+          ((s)++);
+        }
+        while (--curw > 0);
+        s = sline;
+        curw =
+          (((w) <
+            ((int) _drawing_x_mask +
+             1)) ? (w) : ((int) _drawing_x_mask + 1));
+      }
+      while (curw > 0);
+    }
+  }
+}