Message ID | 20150411163443.GE19273@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
On Sat, 11 Apr 2015, Jakub Jelinek wrote: > @@ -1391,13 +1391,13 @@ thread_through_normal_block (edge e, > vec<basic_block, va_gc> *bb_path; > vec_alloc (bb_path, n_basic_blocks_for_fn (cfun)); > vec_safe_push (bb_path, e->dest); > - hash_set<gimple> *visited_phis = new hash_set<gimple>; > + hash_set<basic_block> *visited_bbs = new hash_set<basic_block>; > > max_threaded_paths = PARAM_VALUE (PARAM_MAX_FSM_THREAD_PATHS); > - fsm_find_control_statement_thread_paths (cond, visited_phis, bb_path, > + fsm_find_control_statement_thread_paths (cond, visited_bbs, bb_path, > false); > > - delete visited_phis; > + delete visited_bbs; > vec_free (bb_path); > } > return 0; I understand minimizing the patches right before the release. At any other time, it would have been a great occasion to remove this new/delete anti-pattern.
On April 11, 2015 6:34:43 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote: >Hi! > >On the following testcase, starting with r221675 aka PR65177 fix >we get ICE, because FSM discovery finds a path that includes the same >blocks >multiple times, like: >Registering FSM jump thread: (9, 4) incoming edge; (4, 5) (5, 12) >(12, 14) (14, 5) (5, 12) nocopy; (5, 12) >All these bbs belong to the same loop, with bb14 being the header and >bb12 >the latch. And the copy_bbs/duplicate_thread_path don't seem to be >really >prepared to duplicate the same basic block more than once. > >fsm_find_control_statement_thread_paths has guard against recursion, >but it >adds to the hash_set the PHI nodes. On the testcase, bb5 is added to >the >path first through one of the PHIs: ># c_3 = PHI <c_39(4), b_17(14)> ># b_33 = PHI <b_32(4), b_17(14)> >and the second time through the other PHI. > >The following patch fixes that by adding to the has_set the basic >blocks >containing the PHIs instead of the PHIs. > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. Thanks, Richard. >2015-04-11 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/65735 > * tree-ssa-threadedge.c (fsm_find_control_statement_thread_paths): > Remove visited_phis argument, add visited_bbs, avoid recursing into >the > same bb rather than just into the same phi node. > (thread_through_normal_block): Adjust caller. > > * gcc.c-torture/compile/pr65735.c: New test. > >--- gcc/tree-ssa-threadedge.c.jj 2015-02-16 22:18:34.000000000 +0100 >+++ gcc/tree-ssa-threadedge.c 2015-04-11 16:13:51.906916300 +0200 >@@ -1015,7 +1015,7 @@ static int max_threaded_paths; > > static void > fsm_find_control_statement_thread_paths (tree expr, >- hash_set<gimple> *visited_phis, >+ hash_set<basic_block> *visited_bbs, > vec<basic_block, va_gc> *&path, > bool seen_loop_phi) > { >@@ -1034,7 +1034,7 @@ fsm_find_control_statement_thread_paths > return; > > /* Avoid infinite recursion. */ >- if (visited_phis->add (def_stmt)) >+ if (visited_bbs->add (var_bb)) > return; > > gphi *phi = as_a <gphi *> (def_stmt); >@@ -1109,7 +1109,7 @@ fsm_find_control_statement_thread_paths > { > vec_safe_push (path, bbi); > /* Recursively follow SSA_NAMEs looking for a constant definition. >*/ >- fsm_find_control_statement_thread_paths (arg, visited_phis, path, >+ fsm_find_control_statement_thread_paths (arg, visited_bbs, path, > seen_loop_phi); > > path->pop (); >@@ -1391,13 +1391,13 @@ thread_through_normal_block (edge e, > vec<basic_block, va_gc> *bb_path; > vec_alloc (bb_path, n_basic_blocks_for_fn (cfun)); > vec_safe_push (bb_path, e->dest); >- hash_set<gimple> *visited_phis = new hash_set<gimple>; >+ hash_set<basic_block> *visited_bbs = new hash_set<basic_block>; > > max_threaded_paths = PARAM_VALUE (PARAM_MAX_FSM_THREAD_PATHS); >- fsm_find_control_statement_thread_paths (cond, visited_phis, >bb_path, >+ fsm_find_control_statement_thread_paths (cond, visited_bbs, >bb_path, > false); > >- delete visited_phis; >+ delete visited_bbs; > vec_free (bb_path); > } > return 0; >--- gcc/testsuite/gcc.c-torture/compile/pr65735.c.jj 2015-04-11 >16:14:33.173263982 +0200 >+++ gcc/testsuite/gcc.c-torture/compile/pr65735.c 2015-04-11 >16:14:06.000000000 +0200 >@@ -0,0 +1,21 @@ >+/* PR tree-optimization/65735 */ >+ >+int foo (void); >+ >+void >+bar (int a, int b, int c) >+{ >+ while (!a) >+ { >+ c = foo (); >+ if (c == 7) >+ c = b; >+ switch (c) >+ { >+ case 1: >+ a = b++; >+ if (b) >+ b = 1; >+ } >+ } >+} > > Jakub
On Sat, Apr 11, 2015 at 07:19:00PM +0200, Marc Glisse wrote: > On Sat, 11 Apr 2015, Jakub Jelinek wrote: > > >@@ -1391,13 +1391,13 @@ thread_through_normal_block (edge e, > > vec<basic_block, va_gc> *bb_path; > > vec_alloc (bb_path, n_basic_blocks_for_fn (cfun)); > > vec_safe_push (bb_path, e->dest); > >- hash_set<gimple> *visited_phis = new hash_set<gimple>; > >+ hash_set<basic_block> *visited_bbs = new hash_set<basic_block>; > > > > max_threaded_paths = PARAM_VALUE (PARAM_MAX_FSM_THREAD_PATHS); > >- fsm_find_control_statement_thread_paths (cond, visited_phis, bb_path, > >+ fsm_find_control_statement_thread_paths (cond, visited_bbs, bb_path, > > false); > > > >- delete visited_phis; > >+ delete visited_bbs; > > vec_free (bb_path); > > } > > return 0; > > I understand minimizing the patches right before the release. At any other > time, it would have been a great occasion to remove this new/delete > anti-pattern. Not at this point, I really wanted to do RC1 on Friday, now it looks more likely for Monday, but really only blocker bugs at this point should go in. Jakub
--- gcc/tree-ssa-threadedge.c.jj 2015-02-16 22:18:34.000000000 +0100 +++ gcc/tree-ssa-threadedge.c 2015-04-11 16:13:51.906916300 +0200 @@ -1015,7 +1015,7 @@ static int max_threaded_paths; static void fsm_find_control_statement_thread_paths (tree expr, - hash_set<gimple> *visited_phis, + hash_set<basic_block> *visited_bbs, vec<basic_block, va_gc> *&path, bool seen_loop_phi) { @@ -1034,7 +1034,7 @@ fsm_find_control_statement_thread_paths return; /* Avoid infinite recursion. */ - if (visited_phis->add (def_stmt)) + if (visited_bbs->add (var_bb)) return; gphi *phi = as_a <gphi *> (def_stmt); @@ -1109,7 +1109,7 @@ fsm_find_control_statement_thread_paths { vec_safe_push (path, bbi); /* Recursively follow SSA_NAMEs looking for a constant definition. */ - fsm_find_control_statement_thread_paths (arg, visited_phis, path, + fsm_find_control_statement_thread_paths (arg, visited_bbs, path, seen_loop_phi); path->pop (); @@ -1391,13 +1391,13 @@ thread_through_normal_block (edge e, vec<basic_block, va_gc> *bb_path; vec_alloc (bb_path, n_basic_blocks_for_fn (cfun)); vec_safe_push (bb_path, e->dest); - hash_set<gimple> *visited_phis = new hash_set<gimple>; + hash_set<basic_block> *visited_bbs = new hash_set<basic_block>; max_threaded_paths = PARAM_VALUE (PARAM_MAX_FSM_THREAD_PATHS); - fsm_find_control_statement_thread_paths (cond, visited_phis, bb_path, + fsm_find_control_statement_thread_paths (cond, visited_bbs, bb_path, false); - delete visited_phis; + delete visited_bbs; vec_free (bb_path); } return 0; --- gcc/testsuite/gcc.c-torture/compile/pr65735.c.jj 2015-04-11 16:14:33.173263982 +0200 +++ gcc/testsuite/gcc.c-torture/compile/pr65735.c 2015-04-11 16:14:06.000000000 +0200 @@ -0,0 +1,21 @@ +/* PR tree-optimization/65735 */ + +int foo (void); + +void +bar (int a, int b, int c) +{ + while (!a) + { + c = foo (); + if (c == 7) + c = b; + switch (c) + { + case 1: + a = b++; + if (b) + b = 1; + } + } +}