| Message ID | e5bef495-b5c0-a853-e672-c1eda5c42e28@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-up EH after strlen transformation (PR tree-optimization/83593). | expand |
On Wed, 3 Jan 2018, Martin Liška wrote: + *cleanup_eh = maybe_clean_or_replace_eh_stmt (stmt, + stmt); Do you mean *cleanup_eh |= ... ?
On Wed, Jan 03, 2018 at 01:27:01PM +0100, Martin Liška wrote: > /* Reading the final '\0' character. */ > tree zero = build_int_cst (TREE_TYPE (lhs), 0); > gimple_set_vuse (stmt, NULL_TREE); > gimple_assign_set_rhs_from_tree (gsi, zero); > + *cleanup_eh = maybe_clean_or_replace_eh_stmt (stmt, > + stmt); The second stmt should be gsi_stmt (*gsi) just in case gimple_assign_set_rhs_from_tree can't modify in-place, and probably you need *cleanup_eh = maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi)); then to get formatting right. > update_stmt (gsi_stmt (*gsi)); > + > + if (dump_file && (dump_flags & TDF_DETAILS) != 0) > + { > + fprintf (dump_file, "into: "); > + print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); Again, should be gsi_stmt (*gsi);. Or do: stmt = gsi_stmt (*gsi); above update_stmt. As stmt is used several times later, I think that is my preference (though, in maybe_clean_or_replace_eh_stmt call you IMHO still want gsi_stmt repeated). > + } > } > else if (w1 > w2) > { > @@ -3399,11 +3416,16 @@ strlen_dom_walker::before_dom_children (basic_block bb) > } > } > > + bool cleanup_eh = false; > + > /* Attempt to optimize individual statements. */ > for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); ) > - if (strlen_check_and_optimize_stmt (&gsi)) > + if (strlen_check_and_optimize_stmt (&gsi, &cleanup_eh)) > gsi_next (&gsi); > > + if (cleanup_eh) > + gimple_purge_dead_eh_edges (bb); If gimple_purge_dead_eh_edges returns true, you want to arrange for the pass to return TODO_cleanup_cfg (probably needs to use some global static variable to propagate that). Jakub
On 01/03/2018 01:49 PM, Marc Glisse wrote: > On Wed, 3 Jan 2018, Martin Liška wrote: > > + *cleanup_eh = maybe_clean_or_replace_eh_stmt (stmt, > + stmt); > > Do you mean *cleanup_eh |= ... ? > Yes. Thanks!
On 01/03/2018 01:50 PM, Jakub Jelinek wrote: > If gimple_purge_dead_eh_edges returns true, you want to arrange for the > pass to return TODO_cleanup_cfg (probably needs to use some global static > variable to propagate that). > > Jakub Hi. Sending v2. I'm suggesting to propagate that in strlen_dom_walker::m_cleanup_cfg. I've just triggered tests. Ready to install after it finishes? From 217982bae6969865051f12798e4da444dd4aaa3f Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Wed, 3 Jan 2018 12:15:40 +0100 Subject: [PATCH] Clean-up EH after strlen transformation (PR tree-optimization/83593). gcc/ChangeLog: 2018-01-03 Martin Liska <mliska@suse.cz> PR tree-optimization/83593 * tree-ssa-strlen.c (strlen_check_and_optimize_stmt): Clean-up EH gimple statements. (strlen_dom_walker::before_dom_children): Call gimple_purge_dead_eh_edges. (pass_strlen::execute): Return TODO_cleanup_cfg if needed. gcc/testsuite/ChangeLog: 2018-01-03 Martin Liska <mliska@suse.cz> PR tree-optimization/83593 * gcc.dg/pr83593.c: New test. --- gcc/testsuite/gcc.dg/pr83593.c | 15 +++++++++++++ gcc/tree-ssa-strlen.c | 48 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 56 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr83593.c diff --git a/gcc/testsuite/gcc.dg/pr83593.c b/gcc/testsuite/gcc.dg/pr83593.c new file mode 100644 index 00000000000..eddecc0606a --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr83593.c @@ -0,0 +1,15 @@ +/* PR tree-optimization/83593 */ +/* { dg-options "-O2 -fno-tree-dominator-opts -fnon-call-exceptions -fno-tree-pre -fexceptions -fno-code-hoisting -fno-tree-fre" } */ + +void +hr (int *ed, signed char *ju) +{ + int kc; + { + int xj; + int *q2 = (*ed == 0) ? &xj : &kc; + + *ju = 0; + kc = *ju; + } +} diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c index be6ab9f1e1b..7ad6ff8228c 100644 --- a/gcc/tree-ssa-strlen.c +++ b/gcc/tree-ssa-strlen.c @@ -39,6 +39,7 @@ along with GCC; see the file COPYING3. If not see #include "gimple-iterator.h" #include "gimplify-me.h" #include "expr.h" +#include "tree-cfg.h" #include "tree-dfa.h" #include "domwalk.h" #include "tree-ssa-alias.h" @@ -3051,10 +3052,12 @@ fold_strstr_to_strncmp (tree rhs1, tree rhs2, gimple *stmt) } /* Attempt to check for validity of the performed access a single statement - at *GSI using string length knowledge, and to optimize it. */ + at *GSI using string length knowledge, and to optimize it. + If the given basic block needs clean-up of EH, CLEANUP_EH is set to + true. */ static bool -strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi) +strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi, bool *cleanup_eh) { gimple *stmt = gsi_stmt (*gsi); @@ -3201,11 +3204,27 @@ strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi) if (w1 == w2 && si->full_string_p) { + if (dump_file && (dump_flags & TDF_DETAILS) != 0) + { + fprintf (dump_file, "Optimizing: "); + print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); + } + /* Reading the final '\0' character. */ tree zero = build_int_cst (TREE_TYPE (lhs), 0); gimple_set_vuse (stmt, NULL_TREE); gimple_assign_set_rhs_from_tree (gsi, zero); - update_stmt (gsi_stmt (*gsi)); + *cleanup_eh + |= maybe_clean_or_replace_eh_stmt (stmt, + gsi_stmt (*gsi)); + stmt = gsi_stmt (*gsi); + update_stmt (stmt); + + if (dump_file && (dump_flags & TDF_DETAILS) != 0) + { + fprintf (dump_file, "into: "); + print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); + } } else if (w1 > w2) { @@ -3318,10 +3337,16 @@ do_invalidate (basic_block dombb, gimple *phi, bitmap visited, int *count) class strlen_dom_walker : public dom_walker { public: - strlen_dom_walker (cdi_direction direction) : dom_walker (direction) {} + strlen_dom_walker (cdi_direction direction) + : dom_walker (direction), m_cleanup_cfg (false) + {} virtual edge before_dom_children (basic_block); virtual void after_dom_children (basic_block); + + /* Flag that will trigger TODO_cleanup_cfg to be returned in strlen + execute function. */ + bool m_cleanup_cfg; }; /* Callback for walk_dominator_tree. Attempt to optimize various @@ -3399,11 +3424,19 @@ strlen_dom_walker::before_dom_children (basic_block bb) } } + bool cleanup_eh = false; + /* Attempt to optimize individual statements. */ for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); ) - if (strlen_check_and_optimize_stmt (&gsi)) + if (strlen_check_and_optimize_stmt (&gsi, &cleanup_eh)) gsi_next (&gsi); + if (cleanup_eh) + { + gimple_purge_dead_eh_edges (bb); + m_cleanup_cfg = true; + } + bb->aux = stridx_to_strinfo; if (vec_safe_length (stridx_to_strinfo) && !strinfo_shared ()) (*stridx_to_strinfo)[0] = (strinfo *) bb; @@ -3477,7 +3510,8 @@ pass_strlen::execute (function *fun) /* String length optimization is implemented as a walk of the dominator tree and a forward walk of statements within each block. */ - strlen_dom_walker (CDI_DOMINATORS).walk (fun->cfg->x_entry_block_ptr); + strlen_dom_walker walker (CDI_DOMINATORS); + walker.walk (fun->cfg->x_entry_block_ptr); ssa_ver_to_stridx.release (); strinfo_pool.release (); @@ -3498,7 +3532,7 @@ pass_strlen::execute (function *fun) strlen_to_stridx = NULL; } - return 0; + return walker.m_cleanup_cfg ? TODO_cleanup_cfg : 0; } } // anon namespace
On Wed, Jan 03, 2018 at 02:07:36PM +0100, Martin Liška wrote: > 2018-01-03 Martin Liska <mliska@suse.cz> > > PR tree-optimization/83593 > * tree-ssa-strlen.c (strlen_check_and_optimize_stmt): Clean-up > EH gimple statements. > (strlen_dom_walker::before_dom_children): Call > gimple_purge_dead_eh_edges. > (pass_strlen::execute): Return TODO_cleanup_cfg if needed. The ChangeLog entry doesn't contain all the changes, like: > @@ -39,6 +39,7 @@ along with GCC; see the file COPYING3. If not see > #include "gimple-iterator.h" > #include "gimplify-me.h" > #include "expr.h" > +#include "tree-cfg.h" > #include "tree-dfa.h" > #include "domwalk.h" > #include "tree-ssa-alias.h" the above one. > static bool > -strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi) > +strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi, bool *cleanup_eh) This one (i.e. addition of a new argument). > @@ -3318,10 +3337,16 @@ do_invalidate (basic_block dombb, gimple *phi, bitmap visited, int *count) > class strlen_dom_walker : public dom_walker > { > public: > - strlen_dom_walker (cdi_direction direction) : dom_walker (direction) {} > + strlen_dom_walker (cdi_direction direction) > + : dom_walker (direction), m_cleanup_cfg (false) > + {} This one. > > virtual edge before_dom_children (basic_block); > virtual void after_dom_children (basic_block); > + > + /* Flag that will trigger TODO_cleanup_cfg to be returned in strlen > + execute function. */ > + bool m_cleanup_cfg; This one too. > + bool cleanup_eh = false; > + > /* Attempt to optimize individual statements. */ > for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); ) > - if (strlen_check_and_optimize_stmt (&gsi)) > + if (strlen_check_and_optimize_stmt (&gsi, &cleanup_eh)) > gsi_next (&gsi); And the fact that strlen_check_and_optimize_stmt caller has been adjusted. > > + if (cleanup_eh) > + { > + gimple_purge_dead_eh_edges (bb); > + m_cleanup_cfg = true; > + } This should be if (cleanup_eh && gimple_purge_dead_eh_edges (bb)) m_cleanup_cfg = true; Ok with that change and updated ChangeLog entry if it passes bootstrap/regtest. Jakub
diff --git a/gcc/testsuite/gcc.dg/pr83593.c b/gcc/testsuite/gcc.dg/pr83593.c new file mode 100644 index 00000000000..eddecc0606a --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr83593.c @@ -0,0 +1,15 @@ +/* PR tree-optimization/83593 */ +/* { dg-options "-O2 -fno-tree-dominator-opts -fnon-call-exceptions -fno-tree-pre -fexceptions -fno-code-hoisting -fno-tree-fre" } */ + +void +hr (int *ed, signed char *ju) +{ + int kc; + { + int xj; + int *q2 = (*ed == 0) ? &xj : &kc; + + *ju = 0; + kc = *ju; + } +} diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c index be6ab9f1e1b..293aeceacce 100644 --- a/gcc/tree-ssa-strlen.c +++ b/gcc/tree-ssa-strlen.c @@ -39,6 +39,7 @@ along with GCC; see the file COPYING3. If not see #include "gimple-iterator.h" #include "gimplify-me.h" #include "expr.h" +#include "tree-cfg.h" #include "tree-dfa.h" #include "domwalk.h" #include "tree-ssa-alias.h" @@ -3051,10 +3052,12 @@ fold_strstr_to_strncmp (tree rhs1, tree rhs2, gimple *stmt) } /* Attempt to check for validity of the performed access a single statement - at *GSI using string length knowledge, and to optimize it. */ + at *GSI using string length knowledge, and to optimize it. + If the given basic block needs clean-up of EH, CLEANUP_EH is set to + true. */ static bool -strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi) +strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi, bool *cleanup_eh) { gimple *stmt = gsi_stmt (*gsi); @@ -3201,11 +3204,25 @@ strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi) if (w1 == w2 && si->full_string_p) { + if (dump_file && (dump_flags & TDF_DETAILS) != 0) + { + fprintf (dump_file, "Optimizing: "); + print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); + } + /* Reading the final '\0' character. */ tree zero = build_int_cst (TREE_TYPE (lhs), 0); gimple_set_vuse (stmt, NULL_TREE); gimple_assign_set_rhs_from_tree (gsi, zero); + *cleanup_eh = maybe_clean_or_replace_eh_stmt (stmt, + stmt); update_stmt (gsi_stmt (*gsi)); + + if (dump_file && (dump_flags & TDF_DETAILS) != 0) + { + fprintf (dump_file, "into: "); + print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); + } } else if (w1 > w2) { @@ -3399,11 +3416,16 @@ strlen_dom_walker::before_dom_children (basic_block bb) } } + bool cleanup_eh = false; + /* Attempt to optimize individual statements. */ for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); ) - if (strlen_check_and_optimize_stmt (&gsi)) + if (strlen_check_and_optimize_stmt (&gsi, &cleanup_eh)) gsi_next (&gsi); + if (cleanup_eh) + gimple_purge_dead_eh_edges (bb); + bb->aux = stridx_to_strinfo; if (vec_safe_length (stridx_to_strinfo) && !strinfo_shared ()) (*stridx_to_strinfo)[0] = (strinfo *) bb;