| Message ID | 20121115200907.GC1886@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 15, 2012 at 9:09 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > On the following testcase substitute_and_fold ICEs because memmove > of length 1 on an empty class is optimized away, and this function wasn't > prepared to handle that. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for > trunk/4.7? I think the bug is that the stmt is removed. fold_stmt is not supposed to "remove" the stmt in any case - it may replace it with a gimple_nop at most. Thus, gimplify_and_update_call_from_tree is at fault here. I am testing a patch that fixes it. Thanks, Richard. > 2012-11-15 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/55331 > * tree-ssa-propagate.c (substitute_and_fold): Handle fold_stmt > turning a stmt into nothing. > > * g++.dg/opt/pr55331.C: New test. > > --- gcc/tree-ssa-propagate.c.jj 2012-11-02 09:01:55.000000000 +0100 > +++ gcc/tree-ssa-propagate.c 2012-11-15 12:27:37.218543417 +0100 > @@ -1094,7 +1094,7 @@ substitute_and_fold (ssa_prop_get_value_ > gimple stmt = gsi_stmt (i); > gimple old_stmt; > enum gimple_code code = gimple_code (stmt); > - gimple_stmt_iterator oldi; > + gimple_stmt_iterator oldi, nexti; > > oldi = i; > if (do_dce) > @@ -1147,6 +1147,8 @@ substitute_and_fold (ssa_prop_get_value_ > } > > old_stmt = stmt; > + nexti = oldi; > + gsi_next (&nexti); > > /* Some statements may be simplified using propagator > specific information. Do this before propagating > @@ -1171,36 +1173,63 @@ substitute_and_fold (ssa_prop_get_value_ > /* Now cleanup. */ > if (did_replace) > { > - stmt = gsi_stmt (oldi); > - > - /* If we cleaned up EH information from the statement, > - remove EH edges. */ > - if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)) > - gimple_purge_dead_eh_edges (bb); > - > - if (is_gimple_assign (stmt) > - && (get_gimple_rhs_class (gimple_assign_rhs_code (stmt)) > - == GIMPLE_SINGLE_RHS)) > - { > - tree rhs = gimple_assign_rhs1 (stmt); > - > - if (TREE_CODE (rhs) == ADDR_EXPR) > - recompute_tree_invariant_for_addr_expr (rhs); > - } > + /* old_stmt might have been also replaced by nothing, > + in that case set stmt to NULL. */ > + if (gsi_end_p (oldi)) > + { > + gcc_checking_assert (gsi_end_p (nexti)); > + stmt = NULL; > + } > + else > + { > + stmt = gsi_stmt (oldi); > + if (!gsi_end_p (nexti) && gsi_stmt (nexti) == stmt) > + stmt = NULL; > + } > > - /* Determine what needs to be done to update the SSA form. */ > - update_stmt (stmt); > - if (!is_gimple_debug (stmt)) > - something_changed = true; > + if (stmt == NULL) > + { > + if (maybe_clean_eh_stmt (old_stmt)) > + gimple_purge_dead_eh_edges (bb); > + something_changed = true; > + } > + else > + { > + /* If we cleaned up EH information from the statement, > + remove EH edges. */ > + if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)) > + gimple_purge_dead_eh_edges (bb); > + > + if (is_gimple_assign (stmt) > + && (get_gimple_rhs_class (gimple_assign_rhs_code (stmt)) > + == GIMPLE_SINGLE_RHS)) > + { > + tree rhs = gimple_assign_rhs1 (stmt); > + > + if (TREE_CODE (rhs) == ADDR_EXPR) > + recompute_tree_invariant_for_addr_expr (rhs); > + } > + > + /* Determine what needs to be done to update the SSA > + form. */ > + update_stmt (stmt); > + if (!is_gimple_debug (stmt)) > + something_changed = true; > + } > } > > if (dump_file && (dump_flags & TDF_DETAILS)) > { > if (did_replace) > { > - fprintf (dump_file, "Folded into: "); > - print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); > - fprintf (dump_file, "\n"); > + if (stmt == NULL) > + fprintf (dump_file, "Folded into nothing\n"); > + else > + { > + fprintf (dump_file, "Folded into: "); > + print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); > + fprintf (dump_file, "\n"); > + } > } > else > fprintf (dump_file, "Not folded\n"); > --- gcc/testsuite/g++.dg/opt/pr55331.C.jj 2012-11-15 12:53:26.893631190 +0100 > +++ gcc/testsuite/g++.dg/opt/pr55331.C 2012-11-15 12:53:52.482504446 +0100 > @@ -0,0 +1,14 @@ > +// PR tree-optimization/55331 > +// { dg-do compile } > +// { dg-options "-O2 -fno-tree-fre" } > + > +struct A {}; > + > +void > +foo (A *p, bool x) > +{ > + A a; > + char *e = (char *) (&a + 1); > + if (x) > + __builtin_memmove (p, &a, e - (char *) &a); > +} > > Jakub
On Mon, Nov 26, 2012 at 04:24:41PM +0100, Richard Biener wrote: > On Thu, Nov 15, 2012 at 9:09 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > On the following testcase substitute_and_fold ICEs because memmove > > of length 1 on an empty class is optimized away, and this function wasn't > > prepared to handle that. > > > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for > > trunk/4.7? > > I think the bug is that the stmt is removed. fold_stmt is not supposed to > "remove" the stmt in any case - it may replace it with a gimple_nop at most. > > Thus, gimplify_and_update_call_from_tree is at fault here. > > I am testing a patch that fixes it. Note that fold_stmt_1 also has code to handle a call folding into nothing, so perhaps that could be removed too if you change it to fold into a nop instead. Jakub
On Mon, Nov 26, 2012 at 4:27 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Nov 26, 2012 at 04:24:41PM +0100, Richard Biener wrote: >> On Thu, Nov 15, 2012 at 9:09 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> > On the following testcase substitute_and_fold ICEs because memmove >> > of length 1 on an empty class is optimized away, and this function wasn't >> > prepared to handle that. >> > >> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for >> > trunk/4.7? >> >> I think the bug is that the stmt is removed. fold_stmt is not supposed to >> "remove" the stmt in any case - it may replace it with a gimple_nop at most. >> >> Thus, gimplify_and_update_call_from_tree is at fault here. >> >> I am testing a patch that fixes it. > > Note that fold_stmt_1 also has code to handle a call folding into nothing, > so perhaps that could be removed too if you change it to fold into a nop > instead. Yeah, I'm testing a patch to remove that. Richard. > Jakub
--- gcc/tree-ssa-propagate.c.jj 2012-11-02 09:01:55.000000000 +0100 +++ gcc/tree-ssa-propagate.c 2012-11-15 12:27:37.218543417 +0100 @@ -1094,7 +1094,7 @@ substitute_and_fold (ssa_prop_get_value_ gimple stmt = gsi_stmt (i); gimple old_stmt; enum gimple_code code = gimple_code (stmt); - gimple_stmt_iterator oldi; + gimple_stmt_iterator oldi, nexti; oldi = i; if (do_dce) @@ -1147,6 +1147,8 @@ substitute_and_fold (ssa_prop_get_value_ } old_stmt = stmt; + nexti = oldi; + gsi_next (&nexti); /* Some statements may be simplified using propagator specific information. Do this before propagating @@ -1171,36 +1173,63 @@ substitute_and_fold (ssa_prop_get_value_ /* Now cleanup. */ if (did_replace) { - stmt = gsi_stmt (oldi); - - /* If we cleaned up EH information from the statement, - remove EH edges. */ - if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)) - gimple_purge_dead_eh_edges (bb); - - if (is_gimple_assign (stmt) - && (get_gimple_rhs_class (gimple_assign_rhs_code (stmt)) - == GIMPLE_SINGLE_RHS)) - { - tree rhs = gimple_assign_rhs1 (stmt); - - if (TREE_CODE (rhs) == ADDR_EXPR) - recompute_tree_invariant_for_addr_expr (rhs); - } + /* old_stmt might have been also replaced by nothing, + in that case set stmt to NULL. */ + if (gsi_end_p (oldi)) + { + gcc_checking_assert (gsi_end_p (nexti)); + stmt = NULL; + } + else + { + stmt = gsi_stmt (oldi); + if (!gsi_end_p (nexti) && gsi_stmt (nexti) == stmt) + stmt = NULL; + } - /* Determine what needs to be done to update the SSA form. */ - update_stmt (stmt); - if (!is_gimple_debug (stmt)) - something_changed = true; + if (stmt == NULL) + { + if (maybe_clean_eh_stmt (old_stmt)) + gimple_purge_dead_eh_edges (bb); + something_changed = true; + } + else + { + /* If we cleaned up EH information from the statement, + remove EH edges. */ + if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)) + gimple_purge_dead_eh_edges (bb); + + if (is_gimple_assign (stmt) + && (get_gimple_rhs_class (gimple_assign_rhs_code (stmt)) + == GIMPLE_SINGLE_RHS)) + { + tree rhs = gimple_assign_rhs1 (stmt); + + if (TREE_CODE (rhs) == ADDR_EXPR) + recompute_tree_invariant_for_addr_expr (rhs); + } + + /* Determine what needs to be done to update the SSA + form. */ + update_stmt (stmt); + if (!is_gimple_debug (stmt)) + something_changed = true; + } } if (dump_file && (dump_flags & TDF_DETAILS)) { if (did_replace) { - fprintf (dump_file, "Folded into: "); - print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); - fprintf (dump_file, "\n"); + if (stmt == NULL) + fprintf (dump_file, "Folded into nothing\n"); + else + { + fprintf (dump_file, "Folded into: "); + print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); + fprintf (dump_file, "\n"); + } } else fprintf (dump_file, "Not folded\n"); --- gcc/testsuite/g++.dg/opt/pr55331.C.jj 2012-11-15 12:53:26.893631190 +0100 +++ gcc/testsuite/g++.dg/opt/pr55331.C 2012-11-15 12:53:52.482504446 +0100 @@ -0,0 +1,14 @@ +// PR tree-optimization/55331 +// { dg-do compile } +// { dg-options "-O2 -fno-tree-fre" } + +struct A {}; + +void +foo (A *p, bool x) +{ + A a; + char *e = (char *) (&a + 1); + if (x) + __builtin_memmove (p, &a, e - (char *) &a); +}