Message ID | 20160913074626.GB7282@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
On Tue, Sep 13, 2016 at 9:46 AM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > The following testcase ICEs because SSA_NAME IMM links are broken. > I've tracked it to DOM's optimize_stmt, a GIMPLE_COND in there is > changed, marked as modified, then in optimize_stmt > if (gimple_modified_p (stmt) || modified_p) > { > tree val = NULL; > > update_stmt_if_modified (stmt); > and a few lines later changed again: > if (gimple_code (stmt) == GIMPLE_COND) > { > if (integer_zerop (val)) > gimple_cond_make_false (as_a <gcond *> (stmt)); > else if (integer_onep (val)) > gimple_cond_make_true (as_a <gcond *> (stmt)); > without update_stmt. As this is a function which update_stmt_if_modified > a few lines before, I think it is fine to update_stmt it immediately. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Ok if it doesn't do the update twice this way (we do it the "fancy way" to only update stmts we modified and do it only once). Richard. > 2016-09-13 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/77454 > * tree-ssa-dom.c (optimize_stmt): Call update_stmt after changing > GIMPLE_COND. Formatting fix. > > * gcc.dg/pr77454.c: New test. > > --- gcc/tree-ssa-dom.c.jj 2016-07-22 15:55:30.000000000 +0200 > +++ gcc/tree-ssa-dom.c 2016-09-09 12:29:28.006188533 +0200 > @@ -1927,8 +1927,9 @@ optimize_stmt (basic_block bb, gimple_st > > if (gimple_code (stmt) == GIMPLE_COND) > val = fold_binary_loc (gimple_location (stmt), > - gimple_cond_code (stmt), boolean_type_node, > - gimple_cond_lhs (stmt), gimple_cond_rhs (stmt)); > + gimple_cond_code (stmt), boolean_type_node, > + gimple_cond_lhs (stmt), > + gimple_cond_rhs (stmt)); > else if (gswitch *swtch_stmt = dyn_cast <gswitch *> (stmt)) > val = gimple_switch_index (swtch_stmt); > > @@ -1946,6 +1947,8 @@ optimize_stmt (basic_block bb, gimple_st > gimple_cond_make_true (as_a <gcond *> (stmt)); > else > gcc_unreachable (); > + > + update_stmt (stmt); > } > > /* Further simplifications may be possible. */ > --- gcc/testsuite/gcc.dg/pr77454.c.jj 2016-09-09 12:34:47.540537483 +0200 > +++ gcc/testsuite/gcc.dg/pr77454.c 2016-09-09 12:32:47.000000000 +0200 > @@ -0,0 +1,28 @@ > +/* PR tree-optimization/77454 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +void > +foo (unsigned char x, char y) > +{ > + while (x != 0) > + { > + unsigned char *a = &x; > + int b; > + > + if (y != 0) > + a = (unsigned char *) &y; > + else if (y + 1 != 0) > + a = (unsigned char *) &y; > + for (x = 0; x < 1; ++x) > + b = 0; > + for (y = 0; y < 3; ++y) > + { > + y = !!y; > + if (y != 0) > + x = y; > + } > + if ((b != 0 ? -1 : *a) < (y = b)) > + b = 1; > + } > +} > > Jakub
On Tue, Sep 13, 2016 at 03:21:47PM +0200, Richard Biener wrote: > > The following testcase ICEs because SSA_NAME IMM links are broken. > > I've tracked it to DOM's optimize_stmt, a GIMPLE_COND in there is > > changed, marked as modified, then in optimize_stmt > > if (gimple_modified_p (stmt) || modified_p) > > { > > tree val = NULL; > > > > update_stmt_if_modified (stmt); > > and a few lines later changed again: > > if (gimple_code (stmt) == GIMPLE_COND) > > { > > if (integer_zerop (val)) > > gimple_cond_make_false (as_a <gcond *> (stmt)); > > else if (integer_onep (val)) > > gimple_cond_make_true (as_a <gcond *> (stmt)); > > without update_stmt. As this is a function which update_stmt_if_modified > > a few lines before, I think it is fine to update_stmt it immediately. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > Ok if it doesn't do the update twice this way (we do it the "fancy way" to > only update stmts we modified and do it only once). It does update it twice this way, the update_stmt_if_modified (stmt); above is done first and then the newly added update_stmt again. There is if (gimple_code (stmt) == GIMPLE_COND) val = fold_binary_loc (gimple_location (stmt), gimple_cond_code (stmt), boolean_type_node, gimple_cond_lhs (stmt), gimple_cond_rhs (stmt)); else if (gswitch *swtch_stmt = dyn_cast <gswitch *> (stmt)) val = gimple_switch_index (swtch_stmt); if (val && TREE_CODE (val) == INTEGER_CST) { retval = find_taken_edge (bb, val); and gimple_cond_make_{false,true} in between. If none of these care about the stmt being updated (gimple_cond_make_{false,true} and gimple_switch_index is surely ok, dyn_cast too, not 100% sure about fold_binary (if it can't use match.pd, though on the other side it is not passed the stmt, but only its arguments), or find_taken_edge (most likely ok)), then I could perhaps change the patch to do gimple_set_modified (stmt, true); where I've added the update_stmt, and move the update_stmt_if_modified call after the if (val && TREE_CODE (val) == INTEGER_CST) { ... } stmt. Jakub
On September 13, 2016 3:32:33 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote: >On Tue, Sep 13, 2016 at 03:21:47PM +0200, Richard Biener wrote: >> > The following testcase ICEs because SSA_NAME IMM links are broken. >> > I've tracked it to DOM's optimize_stmt, a GIMPLE_COND in there is >> > changed, marked as modified, then in optimize_stmt >> > if (gimple_modified_p (stmt) || modified_p) >> > { >> > tree val = NULL; >> > >> > update_stmt_if_modified (stmt); >> > and a few lines later changed again: >> > if (gimple_code (stmt) == GIMPLE_COND) >> > { >> > if (integer_zerop (val)) >> > gimple_cond_make_false (as_a <gcond *> (stmt)); >> > else if (integer_onep (val)) >> > gimple_cond_make_true (as_a <gcond *> (stmt)); >> > without update_stmt. As this is a function which >update_stmt_if_modified >> > a few lines before, I think it is fine to update_stmt it >immediately. >> > >> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for >trunk? >> >> Ok if it doesn't do the update twice this way (we do it the "fancy >way" to >> only update stmts we modified and do it only once). > >It does update it twice this way, the > update_stmt_if_modified (stmt); >above is done first and then the newly added update_stmt again. > >There is > if (gimple_code (stmt) == GIMPLE_COND) > val = fold_binary_loc (gimple_location (stmt), > gimple_cond_code (stmt), boolean_type_node, > gimple_cond_lhs (stmt), gimple_cond_rhs (stmt)); > else if (gswitch *swtch_stmt = dyn_cast <gswitch *> (stmt)) > val = gimple_switch_index (swtch_stmt); > > if (val && TREE_CODE (val) == INTEGER_CST) > { > retval = find_taken_edge (bb, val); >and gimple_cond_make_{false,true} in between. If none of these care >about >the stmt being updated (gimple_cond_make_{false,true} and >gimple_switch_index is surely ok, dyn_cast too, not 100% sure about >fold_binary (if it can't use match.pd, though on the other side it is >not >passed the stmt, but only its arguments), or find_taken_edge (most >likely >ok)), then I could perhaps change the patch to do gimple_set_modified >(stmt, >true); where I've added the update_stmt, and move the >update_stmt_if_modified call after the if (val && TREE_CODE (val) == >INTEGER_CST) { ... } stmt. Yes please, this should be safe. All of the above do not look at immediate uses but at most use-def links. Richard. > Jakub
--- gcc/tree-ssa-dom.c.jj 2016-07-22 15:55:30.000000000 +0200 +++ gcc/tree-ssa-dom.c 2016-09-09 12:29:28.006188533 +0200 @@ -1927,8 +1927,9 @@ optimize_stmt (basic_block bb, gimple_st if (gimple_code (stmt) == GIMPLE_COND) val = fold_binary_loc (gimple_location (stmt), - gimple_cond_code (stmt), boolean_type_node, - gimple_cond_lhs (stmt), gimple_cond_rhs (stmt)); + gimple_cond_code (stmt), boolean_type_node, + gimple_cond_lhs (stmt), + gimple_cond_rhs (stmt)); else if (gswitch *swtch_stmt = dyn_cast <gswitch *> (stmt)) val = gimple_switch_index (swtch_stmt); @@ -1946,6 +1947,8 @@ optimize_stmt (basic_block bb, gimple_st gimple_cond_make_true (as_a <gcond *> (stmt)); else gcc_unreachable (); + + update_stmt (stmt); } /* Further simplifications may be possible. */ --- gcc/testsuite/gcc.dg/pr77454.c.jj 2016-09-09 12:34:47.540537483 +0200 +++ gcc/testsuite/gcc.dg/pr77454.c 2016-09-09 12:32:47.000000000 +0200 @@ -0,0 +1,28 @@ +/* PR tree-optimization/77454 */ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +void +foo (unsigned char x, char y) +{ + while (x != 0) + { + unsigned char *a = &x; + int b; + + if (y != 0) + a = (unsigned char *) &y; + else if (y + 1 != 0) + a = (unsigned char *) &y; + for (x = 0; x < 1; ++x) + b = 0; + for (y = 0; y < 3; ++y) + { + y = !!y; + if (y != 0) + x = y; + } + if ((b != 0 ? -1 : *a) < (y = b)) + b = 1; + } +}