| Message ID | 50239B68.1010208@ispras.ru |
|---|---|
| State | New |
| Headers | show |
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
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); }
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
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); + } + } +}