| Message ID | 20111212170149.GS1957@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/12/2011 09:01 AM, Jakub Jelinek wrote: > 2011-12-12 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/51481 > * gimple-fold.c (gimple_fold_call): Call > maybe_clean_or_replace_eh_stmt. Avoid optimization if stmt has EH > edges, but gimple_fold_builtin result can't throw. > > * gcc.dg/pr51481.c: New test. Ok. r~
On Mon, Dec 12, 2011 at 6:01 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg > cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when > we've replaced a printf call (which can throw) with puts (which can throw > too), nothing would update EH stmts. It would be problematic calling > gimple_purge_dead_eh_edges, because callers might be surprised by that, > especially when this happens during cfg cleanup, so instead I just assert > it is not needed and don't try to fold if a throwing stmt would be replaced > by non-throwing. FAB pass can handle that instead. No folding > has been actually disabled because of that check during bootstrap/regtest, > so it is there just in case. I think that all callers of fold_stmt are supposed to handle EH updating themselves, similar to how they are supposed to call update_stmt. I see that replace_uses_by does call maybe_clean_or_replace_eh_stmt - are you sure it is this path the issue triggers on? Richard. > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2011-12-12 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/51481 > * gimple-fold.c (gimple_fold_call): Call > maybe_clean_or_replace_eh_stmt. Avoid optimization if stmt has EH > edges, but gimple_fold_builtin result can't throw. > > * gcc.dg/pr51481.c: New test. > > --- gcc/gimple-fold.c.jj 2011-12-11 22:02:37.000000000 +0100 > +++ gcc/gimple-fold.c 2011-12-12 11:42:34.740168390 +0100 > @@ -1117,10 +1117,21 @@ gimple_fold_call (gimple_stmt_iterator * > if (callee && DECL_BUILT_IN (callee)) > { > tree result = gimple_fold_builtin (stmt); > - if (result) > + if (result > + /* Disallow EH edge removal here. We can't call > + gimple_purge_dead_eh_edges here. */ > + && (lookup_stmt_eh_lp (stmt) == 0 > + || tree_could_throw_p (result))) > { > if (!update_call_from_tree (gsi, result)) > gimplify_and_update_call_from_tree (gsi, result); > + if (!gsi_end_p (*gsi)) > + { > + gimple new_stmt = gsi_stmt (*gsi); > + bool update_eh ATTRIBUTE_UNUSED > + = maybe_clean_or_replace_eh_stmt (stmt, new_stmt); > + gcc_assert (!update_eh); > + } > changed = true; > } > } > --- gcc/testsuite/gcc.dg/pr51481.c.jj 2011-12-12 11:18:27.304678207 +0100 > +++ gcc/testsuite/gcc.dg/pr51481.c 2011-12-12 11:18:02.000000000 +0100 > @@ -0,0 +1,33 @@ > +/* PR tree-optimization/51481 */ > +/* { dg-do compile } */ > +/* { dg-options "-O -fexceptions -fipa-cp -fipa-cp-clone" } */ > + > +extern const unsigned short int **foo (void) > + __attribute__ ((__nothrow__, __const__)); > +struct S { unsigned short s1; int s2; }; > +extern struct S *s[26]; > + > +void > +bar (int x, struct S *y, ...) > +{ > + static struct S *t; > + __builtin_va_list ap; > + __builtin_va_start (ap, y); > + if (t != s[7]) > + { > + const char *p = "aAbBc"; > + t = s[7]; > + while ((*foo ())[(unsigned char) *p]) > + p++; > + } > + __builtin_printf (x == 0 ? "abc\n" : "def\n"); > + if (y != 0) > + __builtin_printf ("ghi %d %d", y->s2, y->s1); > + __builtin_va_end (ap); > +} > + > +void > +baz (char *x) > +{ > + bar (1, 0, x); > +} > > Jakub
On Tue, Dec 13, 2011 at 11:52:38AM +0100, Richard Guenther wrote: > On Mon, Dec 12, 2011 at 6:01 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg > > cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when > > we've replaced a printf call (which can throw) with puts (which can throw > > too), nothing would update EH stmts. It would be problematic calling > > gimple_purge_dead_eh_edges, because callers might be surprised by that, > > especially when this happens during cfg cleanup, so instead I just assert > > it is not needed and don't try to fold if a throwing stmt would be replaced > > by non-throwing. FAB pass can handle that instead. No folding > > has been actually disabled because of that check during bootstrap/regtest, > > so it is there just in case. Note, I've already committed the patch yesterday. > I think that all callers of fold_stmt are supposed to handle EH > updating themselves, similar to how they are supposed to > call update_stmt. I see that replace_uses_by does call Some do something, but others don't. Do you think it is preferable when the callers do that (all of them)? Even if that is chosen, while some could purge dead eh edges, other places can't do that IMHO, so either we need to simply not do transformations that remove EH edges in fold_stmt, or have an argument that is passed down whether it is ok to do so. > maybe_clean_or_replace_eh_stmt - are you sure it is this path > the issue triggers on? Yes, I'm sure this was in replace_uses_by. Apparently it calls maybe_clean_or_replace_eh_stmt, but incorrectly (similarly forwprop in some cases doesn't call it at all, in other places incorrectly): maybe_clean_or_replace_eh_stmt (stmt, stmt); This does nothing. It must be called with the old stmt and new stmt it was replaced with, otherwise, when it is called just with new stmt as in this place lookup_stmt_eh_lp will just return 0 (unless fold_stmt did call it already properly). Jakub
On Tue, Dec 13, 2011 at 1:00 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Dec 13, 2011 at 11:52:38AM +0100, Richard Guenther wrote: >> On Mon, Dec 12, 2011 at 6:01 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> > In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg >> > cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when >> > we've replaced a printf call (which can throw) with puts (which can throw >> > too), nothing would update EH stmts. It would be problematic calling >> > gimple_purge_dead_eh_edges, because callers might be surprised by that, >> > especially when this happens during cfg cleanup, so instead I just assert >> > it is not needed and don't try to fold if a throwing stmt would be replaced >> > by non-throwing. FAB pass can handle that instead. No folding >> > has been actually disabled because of that check during bootstrap/regtest, >> > so it is there just in case. > > Note, I've already committed the patch yesterday. > >> I think that all callers of fold_stmt are supposed to handle EH >> updating themselves, similar to how they are supposed to >> call update_stmt. I see that replace_uses_by does call > > Some do something, but others don't. Do you think it is preferable when > the callers do that (all of them)? Even if that is chosen, while some could > purge dead eh edges, other places can't do that IMHO, so either we need to > simply not do transformations that remove EH edges in fold_stmt, or have an > argument that is passed down whether it is ok to do so. > >> maybe_clean_or_replace_eh_stmt - are you sure it is this path >> the issue triggers on? > > Yes, I'm sure this was in replace_uses_by. Apparently it calls > maybe_clean_or_replace_eh_stmt, but incorrectly (similarly forwprop in some > cases doesn't call it at all, in other places incorrectly): > > maybe_clean_or_replace_eh_stmt (stmt, stmt); > > This does nothing. It must be called with the old stmt and new stmt it was > replaced with, otherwise, when it is called just with new stmt as in this > place lookup_stmt_eh_lp will just return 0 (unless fold_stmt did call it already > properly). Yeah, I'm testing a followup patch that fixes the call in replace_uses_by (that alone fixes the testcase in the PR). Richard. > Jakub
On Tue, Dec 13, 2011 at 1:00 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Dec 13, 2011 at 11:52:38AM +0100, Richard Guenther wrote: >> On Mon, Dec 12, 2011 at 6:01 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> > In gimple_fold_call (called from fold_stmt from replace_uses_by from cfg >> > cleanup) we weren't calling maybe_clean_or_replace_eh_stmt, so when >> > we've replaced a printf call (which can throw) with puts (which can throw >> > too), nothing would update EH stmts. It would be problematic calling >> > gimple_purge_dead_eh_edges, because callers might be surprised by that, >> > especially when this happens during cfg cleanup, so instead I just assert >> > it is not needed and don't try to fold if a throwing stmt would be replaced >> > by non-throwing. FAB pass can handle that instead. No folding >> > has been actually disabled because of that check during bootstrap/regtest, >> > so it is there just in case. > > Note, I've already committed the patch yesterday. Yes, I've seen that. I think it's ugly and inconsistent - we can certaily fold non-call stmts from throwing to non-throwing. Think of -fnon-call-exceptions and x + 1.3 and folding after propagating 1.0 to x. It is not fold_stmts business to care about EH info or edges (it works on statements in isolation). >> I think that all callers of fold_stmt are supposed to handle EH >> updating themselves, similar to how they are supposed to >> call update_stmt. I see that replace_uses_by does call > > Some do something, but others don't. Do you think it is preferable when > the callers do that (all of them)? Even if that is chosen, while some could > purge dead eh edges, other places can't do that IMHO, so either we need to > simply not do transformations that remove EH edges in fold_stmt, or have an > argument that is passed down whether it is ok to do so. I think all callers can (should be able to) do that. In the light of -fnon-call-exceptions we probably miss quite a few - but we should fix them. [Unfortunately some of our propagation helpers do not, or do not have a good way of returning whether EH/CFG cleanup is necessary, notably gsi_remove, which removes the stmt from the EH tables but does not purge edges.] Richard.
On Tue, Dec 13, 2011 at 01:05:30PM +0100, Richard Guenther wrote: > Yeah, I'm testing a followup patch that fixes the call in replace_uses_by > (that alone fixes the testcase in the PR). That patch looks good, but then forwprop has the same static void tidy_after_forward_propagate_addr (gimple stmt) { /* We may have turned a trapping insn into a non-trapping insn. */ if (maybe_clean_or_replace_eh_stmt (stmt, stmt) && gimple_purge_dead_eh_edges (gimple_bb (stmt))) cfg_changed = true; ... Other spots are maybe ok, so if you want, the maybe_cleanup_or_replace_eh_stmt call from gimple_fold_call can be nuked afterwards. But what about that lookup_*/tree_could_throw_p check? Should it stay? Jakub
On Tue, Dec 13, 2011 at 1:15 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Dec 13, 2011 at 01:05:30PM +0100, Richard Guenther wrote: >> Yeah, I'm testing a followup patch that fixes the call in replace_uses_by >> (that alone fixes the testcase in the PR). > > That patch looks good, but then forwprop has the same > static void > tidy_after_forward_propagate_addr (gimple stmt) > { > /* We may have turned a trapping insn into a non-trapping insn. */ > if (maybe_clean_or_replace_eh_stmt (stmt, stmt) > && gimple_purge_dead_eh_edges (gimple_bb (stmt))) > cfg_changed = true; > ... > > Other spots are maybe ok, so if you want, the > maybe_cleanup_or_replace_eh_stmt call from gimple_fold_call can be nuked > afterwards. But what about that lookup_*/tree_could_throw_p check? > Should it stay? I don't think so, if we want to prevent fold_stmt from running into such situation we should do it centrally (similar to the in-place variant). But I wouldn't go down this route without further evidence (read: a testcase that we cannot fix otherwise). Richard. > Jakub
--- gcc/gimple-fold.c.jj 2011-12-11 22:02:37.000000000 +0100 +++ gcc/gimple-fold.c 2011-12-12 11:42:34.740168390 +0100 @@ -1117,10 +1117,21 @@ gimple_fold_call (gimple_stmt_iterator * if (callee && DECL_BUILT_IN (callee)) { tree result = gimple_fold_builtin (stmt); - if (result) + if (result + /* Disallow EH edge removal here. We can't call + gimple_purge_dead_eh_edges here. */ + && (lookup_stmt_eh_lp (stmt) == 0 + || tree_could_throw_p (result))) { if (!update_call_from_tree (gsi, result)) gimplify_and_update_call_from_tree (gsi, result); + if (!gsi_end_p (*gsi)) + { + gimple new_stmt = gsi_stmt (*gsi); + bool update_eh ATTRIBUTE_UNUSED + = maybe_clean_or_replace_eh_stmt (stmt, new_stmt); + gcc_assert (!update_eh); + } changed = true; } } --- gcc/testsuite/gcc.dg/pr51481.c.jj 2011-12-12 11:18:27.304678207 +0100 +++ gcc/testsuite/gcc.dg/pr51481.c 2011-12-12 11:18:02.000000000 +0100 @@ -0,0 +1,33 @@ +/* PR tree-optimization/51481 */ +/* { dg-do compile } */ +/* { dg-options "-O -fexceptions -fipa-cp -fipa-cp-clone" } */ + +extern const unsigned short int **foo (void) + __attribute__ ((__nothrow__, __const__)); +struct S { unsigned short s1; int s2; }; +extern struct S *s[26]; + +void +bar (int x, struct S *y, ...) +{ + static struct S *t; + __builtin_va_list ap; + __builtin_va_start (ap, y); + if (t != s[7]) + { + const char *p = "aAbBc"; + t = s[7]; + while ((*foo ())[(unsigned char) *p]) + p++; + } + __builtin_printf (x == 0 ? "abc\n" : "def\n"); + if (y != 0) + __builtin_printf ("ghi %d %d", y->s2, y->s1); + __builtin_va_end (ap); +} + +void +baz (char *x) +{ + bar (1, 0, x); +}