| Message ID | ri6sfc1g1lx.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | ipa: Self-DCE of uses of removed call LHSs (PR 108007) | expand |
On 12 May 2023 14:45:14 CEST, Martin Jambor <mjambor@suse.cz> wrote: >gcc/ChangeLog: > >2023-05-11 Martin Jambor <mjambor@suse.cz> > > PR ipa/108007 > * cgraph.h (cgraph_edge): Add a parameter to > redirect_call_stmt_to_callee. > * ipa-param-manipulation.h (ipa_param_adjustments): Added a > parameter to modify_call. > * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New > parameter killed_ssas, pass it to padjs->modify_call. > * ipa-param-manipulation.cc (purge_transitive_uses): New function. > (ipa_param_adjustments::modify_call): New parameter killed_ssas. > Instead of substitutin uses, invoke purge_transitive_uses. If > hash of killed SSAs has not been provided, create a temporary one > and release SSAs that have been added to it. > * tree-inline.cc (redirect_all_calls): Create > id->killed_new_ssa_names earlier, pass it to edge redirection, > adjust a comment. > (copy_body): Release SSAs in id->killed_new_ssa_names. Nit: Please use present tense in the ChangeLog. s/Added/Add/ And there is a trailing 'g' missing in substitutin thanks,
Ping. Thanks, Martin On Fri, May 12 2023, Martin Jambor wrote: > Hi, > > PR 108007 is another manifestation where we rely on DCE to clean-up > after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA > can leave behind statements which are fed uninitialized values and > trap, even though their results are themselves never used. > > I have already fixed this for unused parameters in callees, this bug > shows that almost the same thing can happen for removed returns, on > the side of callers. This means that the issue has to be fixed > elsewhere, in call redirection. This patch adds a function which > recursivewly looks for uses of operations fed specific SSA names and > removes them all. > > That would have been easy if it wasn't for debug statements during > tree-inline (from which call redirection is also invoked). Debug > statements are decoupled from the rest at this point and iterating > over uses of SSAs does not bring them up. During tree-inline they are > handled especially at the end, I assume in order to make sure that > relative ordering of UIDs are the same with and without debug info. > > This means that during tree-inline we need to make a hash of killed > SSAs, that we already have in copy_body_data, available to the > function making the purging. So the patch duly does also that, making > the interface slightly ugly. > > Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure > the problem is grave enough to warrant backporting to release branches > but can do that as well if people think I should.) > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2023-05-11 Martin Jambor <mjambor@suse.cz> > > PR ipa/108007 > * cgraph.h (cgraph_edge): Add a parameter to > redirect_call_stmt_to_callee. > * ipa-param-manipulation.h (ipa_param_adjustments): Added a > parameter to modify_call. > * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New > parameter killed_ssas, pass it to padjs->modify_call. > * ipa-param-manipulation.cc (purge_transitive_uses): New function. > (ipa_param_adjustments::modify_call): New parameter killed_ssas. > Instead of substitutin uses, invoke purge_transitive_uses. If > hash of killed SSAs has not been provided, create a temporary one > and release SSAs that have been added to it. > * tree-inline.cc (redirect_all_calls): Create > id->killed_new_ssa_names earlier, pass it to edge redirection, > adjust a comment. > (copy_body): Release SSAs in id->killed_new_ssa_names. > > gcc/testsuite/ChangeLog: > > 2023-05-11 Martin Jambor <mjambor@suse.cz> > > PR ipa/108007 > * gcc.dg/ipa/pr108007.c: New test. > --- > gcc/cgraph.cc | 10 +++- > gcc/cgraph.h | 9 ++- > gcc/ipa-param-manipulation.cc | 85 +++++++++++++++++++++-------- > gcc/ipa-param-manipulation.h | 3 +- > gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++++++++++ > gcc/tree-inline.cc | 28 ++++++---- > 6 files changed, 129 insertions(+), 38 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c > > diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc > index e8f9bec8227..5e923bf0557 100644 > --- a/gcc/cgraph.cc > +++ b/gcc/cgraph.cc > @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n) > speculative indirect call, remove "speculative" of the indirect call and > also redirect stmt to it's final direct target. > > + When called from within tree-inline, KILLED_SSAs has to contain the pointer > + to killed_new_ssa_names within the copy_body_data structure and SSAs > + discovered to be useless (if LHS is removed) will be added to it, otherwise > + it needs to be NULL. > + > It is up to caller to iteratively transform each "speculative" > direct call as appropriate. */ > > gimple * > -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) > +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e, > + hash_set <tree> *killed_ssas) > { > tree decl = gimple_call_fndecl (e->call_stmt); > gcall *new_stmt; > @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) > remove_stmt_from_eh_lp (e->call_stmt); > > tree old_fntype = gimple_call_fntype (e->call_stmt); > - new_stmt = padjs->modify_call (e, false); > + new_stmt = padjs->modify_call (e, false, killed_ssas); > cgraph_node *origin = e->callee; > while (origin->clone_of) > origin = origin->clone_of; > diff --git a/gcc/cgraph.h b/gcc/cgraph.h > index f5f54769eda..c1a3691b6f5 100644 > --- a/gcc/cgraph.h > +++ b/gcc/cgraph.h > @@ -1833,9 +1833,16 @@ public: > speculative indirect call, remove "speculative" of the indirect call and > also redirect stmt to it's final direct target. > > + When called from within tree-inline, KILLED_SSAs has to contain the > + pointer to killed_new_ssa_names within the copy_body_data structure and > + SSAs discovered to be useless (if LHS is removed) will be added to it, > + otherwise it needs to be NULL. > + > It is up to caller to iteratively transform each "speculative" > direct call as appropriate. */ > - static gimple *redirect_call_stmt_to_callee (cgraph_edge *e); > + static gimple *redirect_call_stmt_to_callee (cgraph_edge *e, > + hash_set <tree> > + *killed_ssas = nullptr); > > /* Create clone of edge in the node N represented > by CALL_EXPR the callgraph. */ > diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc > index a286af7f5d9..bf2adeb4bd6 100644 > --- a/gcc/ipa-param-manipulation.cc > +++ b/gcc/ipa-param-manipulation.cc > @@ -593,14 +593,63 @@ isra_get_ref_base_and_offset (tree expr, tree *base_p, unsigned *unit_offset_p) > return true; > } > > +/* Remove all statements that use NAME and transitively those that use the > + result of such statements. KILLED_SSAS contains the SSA_NAMEs that are > + already being or have been processed and new ones need to be added to it. > + The funtction only has to process situations handled by > + ssa_name_only_returned_p in ipa-sra.cc with the exception that it can assume > + it must never reach a use in a return statement. */ > + > +static void > +purge_transitive_uses (tree name, hash_set <tree> *killed_ssas) > +{ > + imm_use_iterator imm_iter; > + gimple *stmt; > + > + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) > + { > + if (gimple_debug_bind_p (stmt)) > + { > + /* When runing within tree-inline, we will never end up here but > + adding the SSAs to killed_ssas will do the trick in this case and > + the respective debug statements will get reset. */ > + > + gimple_debug_bind_reset_value (stmt); > + update_stmt (stmt); > + continue; > + } > + > + tree lhs = NULL_TREE; > + if (is_gimple_assign (stmt)) > + lhs = gimple_assign_lhs (stmt); > + else if (gimple_code (stmt) == GIMPLE_PHI) > + lhs = gimple_phi_result (stmt); > + gcc_assert (lhs > + && (TREE_CODE (lhs) == SSA_NAME) > + && !gimple_vdef (stmt)); > + > + if (!killed_ssas->contains (lhs)) > + { > + killed_ssas->add (lhs); > + purge_transitive_uses (lhs, killed_ssas); > + gimple_stmt_iterator gsi = gsi_for_stmt (stmt); > + gsi_remove (&gsi, true); > + } > + } > +} > + > /* Modify actual arguments of a function call in statement currently belonging > to CS, and make it call CS->callee->decl. Return the new statement that > replaced the old one. When invoked, cfun and current_function_decl have to > - be set to the caller. */ > + be set to the caller. When called from within tree-inline, KILLED_SSAs has > + to contain the pointer to killed_new_ssa_names within the copy_body_data > + structure and SSAs discovered to be useless (if LHS is removed) will be > + added to it, otherwise it needs to be NULL. */ > > gcall * > ipa_param_adjustments::modify_call (cgraph_edge *cs, > - bool update_references) > + bool update_references, > + hash_set <tree> *killed_ssas) > { > gcall *stmt = cs->call_stmt; > tree callee_decl = cs->callee->decl; > @@ -910,32 +959,20 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, > > gcall *new_stmt = gimple_build_call_vec (callee_decl, vargs); > > - tree ssa_to_remove = NULL; > + hash_set <tree> *ssas_to_remove = NULL; > if (tree lhs = gimple_call_lhs (stmt)) > { > if (!m_skip_return) > gimple_call_set_lhs (new_stmt, lhs); > else if (TREE_CODE (lhs) == SSA_NAME) > { > - /* LHS should now by a default-def SSA. Unfortunately default-def > - SSA_NAMEs need a backing variable (or at least some code examining > - SSAs assumes it is non-NULL). So we either have to re-use the > - decl we have at hand or introdice a new one. */ > - tree repl = create_tmp_var (TREE_TYPE (lhs), "removed_return"); > - repl = get_or_create_ssa_default_def (cfun, repl); > - SSA_NAME_IS_DEFAULT_DEF (repl) = true; > - imm_use_iterator ui; > - use_operand_p use_p; > - gimple *using_stmt; > - FOR_EACH_IMM_USE_STMT (using_stmt, ui, lhs) > + if (!killed_ssas) > { > - FOR_EACH_IMM_USE_ON_STMT (use_p, ui) > - { > - SET_USE (use_p, repl); > - } > - update_stmt (using_stmt); > + ssas_to_remove = new hash_set<tree> (8); > + killed_ssas = ssas_to_remove; > } > - ssa_to_remove = lhs; > + killed_ssas->add (lhs); > + purge_transitive_uses (lhs, killed_ssas); > } > } > > @@ -954,8 +991,12 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, > fprintf (dump_file, "\n"); > } > gsi_replace (&gsi, new_stmt, true); > - if (ssa_to_remove) > - release_ssa_name (ssa_to_remove); > + if (ssas_to_remove) > + { > + for (tree sn : *ssas_to_remove) > + release_ssa_name (sn); > + delete ssas_to_remove; > + } > if (update_references) > do > { > diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h > index 9798eedf0b6..5b2f90f8307 100644 > --- a/gcc/ipa-param-manipulation.h > +++ b/gcc/ipa-param-manipulation.h > @@ -224,7 +224,8 @@ public: > > /* Modify a call statement arguments (and possibly remove the return value) > as described in the data fields of this class. */ > - gcall *modify_call (cgraph_edge *cs, bool update_references); > + gcall *modify_call (cgraph_edge *cs, bool update_references, > + hash_set <tree> *killed_ssas); > /* Return if the first parameter is left intact. */ > bool first_param_intact_p (); > /* Build a function type corresponding to the modified call. */ > diff --git a/gcc/testsuite/gcc.dg/ipa/pr108007.c b/gcc/testsuite/gcc.dg/ipa/pr108007.c > new file mode 100644 > index 00000000000..77fc95975cf > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr108007.c > @@ -0,0 +1,32 @@ > +/* { dg-do run } */ > +/* { dg-options "-Os -fno-dce -fno-tree-dce -g" } */ > + > +/* This tests that when IPA-SRA removes a LHS of a call statement which, in the > + original source, is fed into a useless operation which however can trap when > + given nonsensical input, that we remove it even when the user has turned off > + normal DCE. */ > + > +int a, b, d, e, f = 10000000, h; > +short c, g; > +static int *i() { > + g = f; > + L: > + h = e = ~g; > + g = ~f % g & e; > + if (!g) > + goto L; > + c++; > + while (g < 1) > + ; > + return &a; > +} > +static void k() { > + int *l, m = 2; > + l = i(); > + for (; d < 1; d++) > + m |= *l >= b; > +} > +int main() { > + k(); > + return 0; > +} > diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc > index 63a19f8d1d8..1c20e9545d1 100644 > --- a/gcc/tree-inline.cc > +++ b/gcc/tree-inline.cc > @@ -2982,20 +2982,19 @@ redirect_all_calls (copy_body_data * id, basic_block bb) > struct cgraph_edge *edge = id->dst_node->get_edge (stmt); > if (edge) > { > + if (!id->killed_new_ssa_names) > + id->killed_new_ssa_names = new hash_set<tree> (16); > gimple *new_stmt > - = cgraph_edge::redirect_call_stmt_to_callee (edge); > - /* If IPA-SRA transformation, run as part of edge redirection, > - removed the LHS because it is unused, save it to > - killed_new_ssa_names so that we can prune it from debug > - statements. */ > + = cgraph_edge::redirect_call_stmt_to_callee (edge, > + id->killed_new_ssa_names); > if (old_lhs > && TREE_CODE (old_lhs) == SSA_NAME > && !gimple_call_lhs (new_stmt)) > - { > - if (!id->killed_new_ssa_names) > - id->killed_new_ssa_names = new hash_set<tree> (16); > - id->killed_new_ssa_names->add (old_lhs); > - } > + /* In case of IPA-SRA removing the LHS, the name should have > + been already added to the hash. But in case of redirecting > + to builtin_unreachable it was not and the name still should > + be pruned from debug statements. */ > + id->killed_new_ssa_names->add (old_lhs); > > if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) > gimple_purge_dead_eh_edges (bb); > @@ -3322,8 +3321,13 @@ copy_body (copy_body_data *id, > body = copy_cfg_body (id, entry_block_map, exit_block_map, > new_entry); > copy_debug_stmts (id); > - delete id->killed_new_ssa_names; > - id->killed_new_ssa_names = NULL; > + if (id->killed_new_ssa_names) > + { > + for (tree sn : *id->killed_new_ssa_names) > + release_ssa_name (sn); > + delete id->killed_new_ssa_names; > + id->killed_new_ssa_names = NULL; > + } > > return body; > } > -- > 2.40.0
On 13 June 2023 17:11:13 CEST, Martin Jambor <mjambor@suse.cz> wrote: >Ping. s/funtction/function/ s/runing/running/ > >Thanks,
Hello and ping. Thanks, Martin On Fri, May 12 2023, Martin Jambor wrote: > Hi, > > PR 108007 is another manifestation where we rely on DCE to clean-up > after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA > can leave behind statements which are fed uninitialized values and > trap, even though their results are themselves never used. > > I have already fixed this for unused parameters in callees, this bug > shows that almost the same thing can happen for removed returns, on > the side of callers. This means that the issue has to be fixed > elsewhere, in call redirection. This patch adds a function which > recursivewly looks for uses of operations fed specific SSA names and > removes them all. > > That would have been easy if it wasn't for debug statements during > tree-inline (from which call redirection is also invoked). Debug > statements are decoupled from the rest at this point and iterating > over uses of SSAs does not bring them up. During tree-inline they are > handled especially at the end, I assume in order to make sure that > relative ordering of UIDs are the same with and without debug info. > > This means that during tree-inline we need to make a hash of killed > SSAs, that we already have in copy_body_data, available to the > function making the purging. So the patch duly does also that, making > the interface slightly ugly. > > Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure > the problem is grave enough to warrant backporting to release branches > but can do that as well if people think I should.) > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2023-05-11 Martin Jambor <mjambor@suse.cz> > > PR ipa/108007 > * cgraph.h (cgraph_edge): Add a parameter to > redirect_call_stmt_to_callee. > * ipa-param-manipulation.h (ipa_param_adjustments): Added a > parameter to modify_call. > * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New > parameter killed_ssas, pass it to padjs->modify_call. > * ipa-param-manipulation.cc (purge_transitive_uses): New function. > (ipa_param_adjustments::modify_call): New parameter killed_ssas. > Instead of substitutin uses, invoke purge_transitive_uses. If > hash of killed SSAs has not been provided, create a temporary one > and release SSAs that have been added to it. > * tree-inline.cc (redirect_all_calls): Create > id->killed_new_ssa_names earlier, pass it to edge redirection, > adjust a comment. > (copy_body): Release SSAs in id->killed_new_ssa_names. > > gcc/testsuite/ChangeLog: > > 2023-05-11 Martin Jambor <mjambor@suse.cz> > > PR ipa/108007 > * gcc.dg/ipa/pr108007.c: New test. > --- > gcc/cgraph.cc | 10 +++- > gcc/cgraph.h | 9 ++- > gcc/ipa-param-manipulation.cc | 85 +++++++++++++++++++++-------- > gcc/ipa-param-manipulation.h | 3 +- > gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++++++++++ > gcc/tree-inline.cc | 28 ++++++---- > 6 files changed, 129 insertions(+), 38 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c > > diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc > index e8f9bec8227..5e923bf0557 100644 > --- a/gcc/cgraph.cc > +++ b/gcc/cgraph.cc > @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n) > speculative indirect call, remove "speculative" of the indirect call and > also redirect stmt to it's final direct target. > > + When called from within tree-inline, KILLED_SSAs has to contain the pointer > + to killed_new_ssa_names within the copy_body_data structure and SSAs > + discovered to be useless (if LHS is removed) will be added to it, otherwise > + it needs to be NULL. > + > It is up to caller to iteratively transform each "speculative" > direct call as appropriate. */ > > gimple * > -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) > +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e, > + hash_set <tree> *killed_ssas) > { > tree decl = gimple_call_fndecl (e->call_stmt); > gcall *new_stmt; > @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) > remove_stmt_from_eh_lp (e->call_stmt); > > tree old_fntype = gimple_call_fntype (e->call_stmt); > - new_stmt = padjs->modify_call (e, false); > + new_stmt = padjs->modify_call (e, false, killed_ssas); > cgraph_node *origin = e->callee; > while (origin->clone_of) > origin = origin->clone_of; > diff --git a/gcc/cgraph.h b/gcc/cgraph.h > index f5f54769eda..c1a3691b6f5 100644 > --- a/gcc/cgraph.h > +++ b/gcc/cgraph.h > @@ -1833,9 +1833,16 @@ public: > speculative indirect call, remove "speculative" of the indirect call and > also redirect stmt to it's final direct target. > > + When called from within tree-inline, KILLED_SSAs has to contain the > + pointer to killed_new_ssa_names within the copy_body_data structure and > + SSAs discovered to be useless (if LHS is removed) will be added to it, > + otherwise it needs to be NULL. > + > It is up to caller to iteratively transform each "speculative" > direct call as appropriate. */ > - static gimple *redirect_call_stmt_to_callee (cgraph_edge *e); > + static gimple *redirect_call_stmt_to_callee (cgraph_edge *e, > + hash_set <tree> > + *killed_ssas = nullptr); > > /* Create clone of edge in the node N represented > by CALL_EXPR the callgraph. */ > diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc > index a286af7f5d9..bf2adeb4bd6 100644 > --- a/gcc/ipa-param-manipulation.cc > +++ b/gcc/ipa-param-manipulation.cc > @@ -593,14 +593,63 @@ isra_get_ref_base_and_offset (tree expr, tree *base_p, unsigned *unit_offset_p) > return true; > } > > +/* Remove all statements that use NAME and transitively those that use the > + result of such statements. KILLED_SSAS contains the SSA_NAMEs that are > + already being or have been processed and new ones need to be added to it. > + The funtction only has to process situations handled by > + ssa_name_only_returned_p in ipa-sra.cc with the exception that it can assume > + it must never reach a use in a return statement. */ > + > +static void > +purge_transitive_uses (tree name, hash_set <tree> *killed_ssas) > +{ > + imm_use_iterator imm_iter; > + gimple *stmt; > + > + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) > + { > + if (gimple_debug_bind_p (stmt)) > + { > + /* When runing within tree-inline, we will never end up here but > + adding the SSAs to killed_ssas will do the trick in this case and > + the respective debug statements will get reset. */ > + > + gimple_debug_bind_reset_value (stmt); > + update_stmt (stmt); > + continue; > + } > + > + tree lhs = NULL_TREE; > + if (is_gimple_assign (stmt)) > + lhs = gimple_assign_lhs (stmt); > + else if (gimple_code (stmt) == GIMPLE_PHI) > + lhs = gimple_phi_result (stmt); > + gcc_assert (lhs > + && (TREE_CODE (lhs) == SSA_NAME) > + && !gimple_vdef (stmt)); > + > + if (!killed_ssas->contains (lhs)) > + { > + killed_ssas->add (lhs); > + purge_transitive_uses (lhs, killed_ssas); > + gimple_stmt_iterator gsi = gsi_for_stmt (stmt); > + gsi_remove (&gsi, true); > + } > + } > +} > + > /* Modify actual arguments of a function call in statement currently belonging > to CS, and make it call CS->callee->decl. Return the new statement that > replaced the old one. When invoked, cfun and current_function_decl have to > - be set to the caller. */ > + be set to the caller. When called from within tree-inline, KILLED_SSAs has > + to contain the pointer to killed_new_ssa_names within the copy_body_data > + structure and SSAs discovered to be useless (if LHS is removed) will be > + added to it, otherwise it needs to be NULL. */ > > gcall * > ipa_param_adjustments::modify_call (cgraph_edge *cs, > - bool update_references) > + bool update_references, > + hash_set <tree> *killed_ssas) > { > gcall *stmt = cs->call_stmt; > tree callee_decl = cs->callee->decl; > @@ -910,32 +959,20 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, > > gcall *new_stmt = gimple_build_call_vec (callee_decl, vargs); > > - tree ssa_to_remove = NULL; > + hash_set <tree> *ssas_to_remove = NULL; > if (tree lhs = gimple_call_lhs (stmt)) > { > if (!m_skip_return) > gimple_call_set_lhs (new_stmt, lhs); > else if (TREE_CODE (lhs) == SSA_NAME) > { > - /* LHS should now by a default-def SSA. Unfortunately default-def > - SSA_NAMEs need a backing variable (or at least some code examining > - SSAs assumes it is non-NULL). So we either have to re-use the > - decl we have at hand or introdice a new one. */ > - tree repl = create_tmp_var (TREE_TYPE (lhs), "removed_return"); > - repl = get_or_create_ssa_default_def (cfun, repl); > - SSA_NAME_IS_DEFAULT_DEF (repl) = true; > - imm_use_iterator ui; > - use_operand_p use_p; > - gimple *using_stmt; > - FOR_EACH_IMM_USE_STMT (using_stmt, ui, lhs) > + if (!killed_ssas) > { > - FOR_EACH_IMM_USE_ON_STMT (use_p, ui) > - { > - SET_USE (use_p, repl); > - } > - update_stmt (using_stmt); > + ssas_to_remove = new hash_set<tree> (8); > + killed_ssas = ssas_to_remove; > } > - ssa_to_remove = lhs; > + killed_ssas->add (lhs); > + purge_transitive_uses (lhs, killed_ssas); > } > } > > @@ -954,8 +991,12 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, > fprintf (dump_file, "\n"); > } > gsi_replace (&gsi, new_stmt, true); > - if (ssa_to_remove) > - release_ssa_name (ssa_to_remove); > + if (ssas_to_remove) > + { > + for (tree sn : *ssas_to_remove) > + release_ssa_name (sn); > + delete ssas_to_remove; > + } > if (update_references) > do > { > diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h > index 9798eedf0b6..5b2f90f8307 100644 > --- a/gcc/ipa-param-manipulation.h > +++ b/gcc/ipa-param-manipulation.h > @@ -224,7 +224,8 @@ public: > > /* Modify a call statement arguments (and possibly remove the return value) > as described in the data fields of this class. */ > - gcall *modify_call (cgraph_edge *cs, bool update_references); > + gcall *modify_call (cgraph_edge *cs, bool update_references, > + hash_set <tree> *killed_ssas); > /* Return if the first parameter is left intact. */ > bool first_param_intact_p (); > /* Build a function type corresponding to the modified call. */ > diff --git a/gcc/testsuite/gcc.dg/ipa/pr108007.c b/gcc/testsuite/gcc.dg/ipa/pr108007.c > new file mode 100644 > index 00000000000..77fc95975cf > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr108007.c > @@ -0,0 +1,32 @@ > +/* { dg-do run } */ > +/* { dg-options "-Os -fno-dce -fno-tree-dce -g" } */ > + > +/* This tests that when IPA-SRA removes a LHS of a call statement which, in the > + original source, is fed into a useless operation which however can trap when > + given nonsensical input, that we remove it even when the user has turned off > + normal DCE. */ > + > +int a, b, d, e, f = 10000000, h; > +short c, g; > +static int *i() { > + g = f; > + L: > + h = e = ~g; > + g = ~f % g & e; > + if (!g) > + goto L; > + c++; > + while (g < 1) > + ; > + return &a; > +} > +static void k() { > + int *l, m = 2; > + l = i(); > + for (; d < 1; d++) > + m |= *l >= b; > +} > +int main() { > + k(); > + return 0; > +} > diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc > index 63a19f8d1d8..1c20e9545d1 100644 > --- a/gcc/tree-inline.cc > +++ b/gcc/tree-inline.cc > @@ -2982,20 +2982,19 @@ redirect_all_calls (copy_body_data * id, basic_block bb) > struct cgraph_edge *edge = id->dst_node->get_edge (stmt); > if (edge) > { > + if (!id->killed_new_ssa_names) > + id->killed_new_ssa_names = new hash_set<tree> (16); > gimple *new_stmt > - = cgraph_edge::redirect_call_stmt_to_callee (edge); > - /* If IPA-SRA transformation, run as part of edge redirection, > - removed the LHS because it is unused, save it to > - killed_new_ssa_names so that we can prune it from debug > - statements. */ > + = cgraph_edge::redirect_call_stmt_to_callee (edge, > + id->killed_new_ssa_names); > if (old_lhs > && TREE_CODE (old_lhs) == SSA_NAME > && !gimple_call_lhs (new_stmt)) > - { > - if (!id->killed_new_ssa_names) > - id->killed_new_ssa_names = new hash_set<tree> (16); > - id->killed_new_ssa_names->add (old_lhs); > - } > + /* In case of IPA-SRA removing the LHS, the name should have > + been already added to the hash. But in case of redirecting > + to builtin_unreachable it was not and the name still should > + be pruned from debug statements. */ > + id->killed_new_ssa_names->add (old_lhs); > > if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) > gimple_purge_dead_eh_edges (bb); > @@ -3322,8 +3321,13 @@ copy_body (copy_body_data *id, > body = copy_cfg_body (id, entry_block_map, exit_block_map, > new_entry); > copy_debug_stmts (id); > - delete id->killed_new_ssa_names; > - id->killed_new_ssa_names = NULL; > + if (id->killed_new_ssa_names) > + { > + for (tree sn : *id->killed_new_ssa_names) > + release_ssa_name (sn); > + delete id->killed_new_ssa_names; > + id->killed_new_ssa_names = NULL; > + } > > return body; > } > -- > 2.40.0
Hello, and ping. Thanks, Martin On Fri, Sep 01 2023, Martin Jambor wrote: > Hello > > and ping. > > Thanks, > > Martin > > > On Fri, May 12 2023, Martin Jambor wrote: >> Hi, >> >> PR 108007 is another manifestation where we rely on DCE to clean-up >> after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA >> can leave behind statements which are fed uninitialized values and >> trap, even though their results are themselves never used. >> >> I have already fixed this for unused parameters in callees, this bug >> shows that almost the same thing can happen for removed returns, on >> the side of callers. This means that the issue has to be fixed >> elsewhere, in call redirection. This patch adds a function which >> recursivewly looks for uses of operations fed specific SSA names and >> removes them all. >> >> That would have been easy if it wasn't for debug statements during >> tree-inline (from which call redirection is also invoked). Debug >> statements are decoupled from the rest at this point and iterating >> over uses of SSAs does not bring them up. During tree-inline they are >> handled especially at the end, I assume in order to make sure that >> relative ordering of UIDs are the same with and without debug info. >> >> This means that during tree-inline we need to make a hash of killed >> SSAs, that we already have in copy_body_data, available to the >> function making the purging. So the patch duly does also that, making >> the interface slightly ugly. >> >> Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure >> the problem is grave enough to warrant backporting to release branches >> but can do that as well if people think I should.) >> >> Thanks, >> >> Martin >> >> >> gcc/ChangeLog: >> >> 2023-05-11 Martin Jambor <mjambor@suse.cz> >> >> PR ipa/108007 >> * cgraph.h (cgraph_edge): Add a parameter to >> redirect_call_stmt_to_callee. >> * ipa-param-manipulation.h (ipa_param_adjustments): Added a >> parameter to modify_call. >> * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New >> parameter killed_ssas, pass it to padjs->modify_call. >> * ipa-param-manipulation.cc (purge_transitive_uses): New function. >> (ipa_param_adjustments::modify_call): New parameter killed_ssas. >> Instead of substitutin uses, invoke purge_transitive_uses. If >> hash of killed SSAs has not been provided, create a temporary one >> and release SSAs that have been added to it. >> * tree-inline.cc (redirect_all_calls): Create >> id->killed_new_ssa_names earlier, pass it to edge redirection, >> adjust a comment. >> (copy_body): Release SSAs in id->killed_new_ssa_names. >> >> gcc/testsuite/ChangeLog: >> >> 2023-05-11 Martin Jambor <mjambor@suse.cz> >> >> PR ipa/108007 >> * gcc.dg/ipa/pr108007.c: New test. >> --- >> gcc/cgraph.cc | 10 +++- >> gcc/cgraph.h | 9 ++- >> gcc/ipa-param-manipulation.cc | 85 +++++++++++++++++++++-------- >> gcc/ipa-param-manipulation.h | 3 +- >> gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++++++++++ >> gcc/tree-inline.cc | 28 ++++++---- >> 6 files changed, 129 insertions(+), 38 deletions(-) >> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c >> >> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc >> index e8f9bec8227..5e923bf0557 100644 >> --- a/gcc/cgraph.cc >> +++ b/gcc/cgraph.cc >> @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n) >> speculative indirect call, remove "speculative" of the indirect call and >> also redirect stmt to it's final direct target. >> >> + When called from within tree-inline, KILLED_SSAs has to contain the pointer >> + to killed_new_ssa_names within the copy_body_data structure and SSAs >> + discovered to be useless (if LHS is removed) will be added to it, otherwise >> + it needs to be NULL. >> + >> It is up to caller to iteratively transform each "speculative" >> direct call as appropriate. */ >> >> gimple * >> -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) >> +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e, >> + hash_set <tree> *killed_ssas) >> { >> tree decl = gimple_call_fndecl (e->call_stmt); >> gcall *new_stmt; >> @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) >> remove_stmt_from_eh_lp (e->call_stmt); >> >> tree old_fntype = gimple_call_fntype (e->call_stmt); >> - new_stmt = padjs->modify_call (e, false); >> + new_stmt = padjs->modify_call (e, false, killed_ssas); >> cgraph_node *origin = e->callee; >> while (origin->clone_of) >> origin = origin->clone_of; >> diff --git a/gcc/cgraph.h b/gcc/cgraph.h >> index f5f54769eda..c1a3691b6f5 100644 >> --- a/gcc/cgraph.h >> +++ b/gcc/cgraph.h >> @@ -1833,9 +1833,16 @@ public: >> speculative indirect call, remove "speculative" of the indirect call and >> also redirect stmt to it's final direct target. >> >> + When called from within tree-inline, KILLED_SSAs has to contain the >> + pointer to killed_new_ssa_names within the copy_body_data structure and >> + SSAs discovered to be useless (if LHS is removed) will be added to it, >> + otherwise it needs to be NULL. >> + >> It is up to caller to iteratively transform each "speculative" >> direct call as appropriate. */ >> - static gimple *redirect_call_stmt_to_callee (cgraph_edge *e); >> + static gimple *redirect_call_stmt_to_callee (cgraph_edge *e, >> + hash_set <tree> >> + *killed_ssas = nullptr); >> >> /* Create clone of edge in the node N represented >> by CALL_EXPR the callgraph. */ >> diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc >> index a286af7f5d9..bf2adeb4bd6 100644 >> --- a/gcc/ipa-param-manipulation.cc >> +++ b/gcc/ipa-param-manipulation.cc >> @@ -593,14 +593,63 @@ isra_get_ref_base_and_offset (tree expr, tree *base_p, unsigned *unit_offset_p) >> return true; >> } >> >> +/* Remove all statements that use NAME and transitively those that use the >> + result of such statements. KILLED_SSAS contains the SSA_NAMEs that are >> + already being or have been processed and new ones need to be added to it. >> + The funtction only has to process situations handled by >> + ssa_name_only_returned_p in ipa-sra.cc with the exception that it can assume >> + it must never reach a use in a return statement. */ >> + >> +static void >> +purge_transitive_uses (tree name, hash_set <tree> *killed_ssas) >> +{ >> + imm_use_iterator imm_iter; >> + gimple *stmt; >> + >> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) >> + { >> + if (gimple_debug_bind_p (stmt)) >> + { >> + /* When runing within tree-inline, we will never end up here but >> + adding the SSAs to killed_ssas will do the trick in this case and >> + the respective debug statements will get reset. */ >> + >> + gimple_debug_bind_reset_value (stmt); >> + update_stmt (stmt); >> + continue; >> + } >> + >> + tree lhs = NULL_TREE; >> + if (is_gimple_assign (stmt)) >> + lhs = gimple_assign_lhs (stmt); >> + else if (gimple_code (stmt) == GIMPLE_PHI) >> + lhs = gimple_phi_result (stmt); >> + gcc_assert (lhs >> + && (TREE_CODE (lhs) == SSA_NAME) >> + && !gimple_vdef (stmt)); >> + >> + if (!killed_ssas->contains (lhs)) >> + { >> + killed_ssas->add (lhs); >> + purge_transitive_uses (lhs, killed_ssas); >> + gimple_stmt_iterator gsi = gsi_for_stmt (stmt); >> + gsi_remove (&gsi, true); >> + } >> + } >> +} >> + >> /* Modify actual arguments of a function call in statement currently belonging >> to CS, and make it call CS->callee->decl. Return the new statement that >> replaced the old one. When invoked, cfun and current_function_decl have to >> - be set to the caller. */ >> + be set to the caller. When called from within tree-inline, KILLED_SSAs has >> + to contain the pointer to killed_new_ssa_names within the copy_body_data >> + structure and SSAs discovered to be useless (if LHS is removed) will be >> + added to it, otherwise it needs to be NULL. */ >> >> gcall * >> ipa_param_adjustments::modify_call (cgraph_edge *cs, >> - bool update_references) >> + bool update_references, >> + hash_set <tree> *killed_ssas) >> { >> gcall *stmt = cs->call_stmt; >> tree callee_decl = cs->callee->decl; >> @@ -910,32 +959,20 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, >> >> gcall *new_stmt = gimple_build_call_vec (callee_decl, vargs); >> >> - tree ssa_to_remove = NULL; >> + hash_set <tree> *ssas_to_remove = NULL; >> if (tree lhs = gimple_call_lhs (stmt)) >> { >> if (!m_skip_return) >> gimple_call_set_lhs (new_stmt, lhs); >> else if (TREE_CODE (lhs) == SSA_NAME) >> { >> - /* LHS should now by a default-def SSA. Unfortunately default-def >> - SSA_NAMEs need a backing variable (or at least some code examining >> - SSAs assumes it is non-NULL). So we either have to re-use the >> - decl we have at hand or introdice a new one. */ >> - tree repl = create_tmp_var (TREE_TYPE (lhs), "removed_return"); >> - repl = get_or_create_ssa_default_def (cfun, repl); >> - SSA_NAME_IS_DEFAULT_DEF (repl) = true; >> - imm_use_iterator ui; >> - use_operand_p use_p; >> - gimple *using_stmt; >> - FOR_EACH_IMM_USE_STMT (using_stmt, ui, lhs) >> + if (!killed_ssas) >> { >> - FOR_EACH_IMM_USE_ON_STMT (use_p, ui) >> - { >> - SET_USE (use_p, repl); >> - } >> - update_stmt (using_stmt); >> + ssas_to_remove = new hash_set<tree> (8); >> + killed_ssas = ssas_to_remove; >> } >> - ssa_to_remove = lhs; >> + killed_ssas->add (lhs); >> + purge_transitive_uses (lhs, killed_ssas); >> } >> } >> >> @@ -954,8 +991,12 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, >> fprintf (dump_file, "\n"); >> } >> gsi_replace (&gsi, new_stmt, true); >> - if (ssa_to_remove) >> - release_ssa_name (ssa_to_remove); >> + if (ssas_to_remove) >> + { >> + for (tree sn : *ssas_to_remove) >> + release_ssa_name (sn); >> + delete ssas_to_remove; >> + } >> if (update_references) >> do >> { >> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h >> index 9798eedf0b6..5b2f90f8307 100644 >> --- a/gcc/ipa-param-manipulation.h >> +++ b/gcc/ipa-param-manipulation.h >> @@ -224,7 +224,8 @@ public: >> >> /* Modify a call statement arguments (and possibly remove the return value) >> as described in the data fields of this class. */ >> - gcall *modify_call (cgraph_edge *cs, bool update_references); >> + gcall *modify_call (cgraph_edge *cs, bool update_references, >> + hash_set <tree> *killed_ssas); >> /* Return if the first parameter is left intact. */ >> bool first_param_intact_p (); >> /* Build a function type corresponding to the modified call. */ >> diff --git a/gcc/testsuite/gcc.dg/ipa/pr108007.c b/gcc/testsuite/gcc.dg/ipa/pr108007.c >> new file mode 100644 >> index 00000000000..77fc95975cf >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/ipa/pr108007.c >> @@ -0,0 +1,32 @@ >> +/* { dg-do run } */ >> +/* { dg-options "-Os -fno-dce -fno-tree-dce -g" } */ >> + >> +/* This tests that when IPA-SRA removes a LHS of a call statement which, in the >> + original source, is fed into a useless operation which however can trap when >> + given nonsensical input, that we remove it even when the user has turned off >> + normal DCE. */ >> + >> +int a, b, d, e, f = 10000000, h; >> +short c, g; >> +static int *i() { >> + g = f; >> + L: >> + h = e = ~g; >> + g = ~f % g & e; >> + if (!g) >> + goto L; >> + c++; >> + while (g < 1) >> + ; >> + return &a; >> +} >> +static void k() { >> + int *l, m = 2; >> + l = i(); >> + for (; d < 1; d++) >> + m |= *l >= b; >> +} >> +int main() { >> + k(); >> + return 0; >> +} >> diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc >> index 63a19f8d1d8..1c20e9545d1 100644 >> --- a/gcc/tree-inline.cc >> +++ b/gcc/tree-inline.cc >> @@ -2982,20 +2982,19 @@ redirect_all_calls (copy_body_data * id, basic_block bb) >> struct cgraph_edge *edge = id->dst_node->get_edge (stmt); >> if (edge) >> { >> + if (!id->killed_new_ssa_names) >> + id->killed_new_ssa_names = new hash_set<tree> (16); >> gimple *new_stmt >> - = cgraph_edge::redirect_call_stmt_to_callee (edge); >> - /* If IPA-SRA transformation, run as part of edge redirection, >> - removed the LHS because it is unused, save it to >> - killed_new_ssa_names so that we can prune it from debug >> - statements. */ >> + = cgraph_edge::redirect_call_stmt_to_callee (edge, >> + id->killed_new_ssa_names); >> if (old_lhs >> && TREE_CODE (old_lhs) == SSA_NAME >> && !gimple_call_lhs (new_stmt)) >> - { >> - if (!id->killed_new_ssa_names) >> - id->killed_new_ssa_names = new hash_set<tree> (16); >> - id->killed_new_ssa_names->add (old_lhs); >> - } >> + /* In case of IPA-SRA removing the LHS, the name should have >> + been already added to the hash. But in case of redirecting >> + to builtin_unreachable it was not and the name still should >> + be pruned from debug statements. */ >> + id->killed_new_ssa_names->add (old_lhs); >> >> if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) >> gimple_purge_dead_eh_edges (bb); >> @@ -3322,8 +3321,13 @@ copy_body (copy_body_data *id, >> body = copy_cfg_body (id, entry_block_map, exit_block_map, >> new_entry); >> copy_debug_stmts (id); >> - delete id->killed_new_ssa_names; >> - id->killed_new_ssa_names = NULL; >> + if (id->killed_new_ssa_names) >> + { >> + for (tree sn : *id->killed_new_ssa_names) >> + release_ssa_name (sn); >> + delete id->killed_new_ssa_names; >> + id->killed_new_ssa_names = NULL; >> + } >> >> return body; >> } >> -- >> 2.40.0
> >> PR 108007 is another manifestation where we rely on DCE to clean-up > >> after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA > >> can leave behind statements which are fed uninitialized values and > >> trap, even though their results are themselves never used. > >> > >> I have already fixed this for unused parameters in callees, this bug > >> shows that almost the same thing can happen for removed returns, on > >> the side of callers. This means that the issue has to be fixed > >> elsewhere, in call redirection. This patch adds a function which > >> recursivewly looks for uses of operations fed specific SSA names and > >> removes them all. > >> > >> That would have been easy if it wasn't for debug statements during > >> tree-inline (from which call redirection is also invoked). Debug > >> statements are decoupled from the rest at this point and iterating > >> over uses of SSAs does not bring them up. During tree-inline they are > >> handled especially at the end, I assume in order to make sure that > >> relative ordering of UIDs are the same with and without debug info. > >> > >> This means that during tree-inline we need to make a hash of killed > >> SSAs, that we already have in copy_body_data, available to the > >> function making the purging. So the patch duly does also that, making > >> the interface slightly ugly. > >> > >> Bootstrapped and tested on x86_64-linux. OK for master? (I am not sure > >> the problem is grave enough to warrant backporting to release branches > >> but can do that as well if people think I should.) > >> > >> Thanks, > >> > >> Martin > >> > >> > >> gcc/ChangeLog: > >> > >> 2023-05-11 Martin Jambor <mjambor@suse.cz> > >> > >> PR ipa/108007 > >> * cgraph.h (cgraph_edge): Add a parameter to > >> redirect_call_stmt_to_callee. > >> * ipa-param-manipulation.h (ipa_param_adjustments): Added a > >> parameter to modify_call. > >> * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New > >> parameter killed_ssas, pass it to padjs->modify_call. > >> * ipa-param-manipulation.cc (purge_transitive_uses): New function. > >> (ipa_param_adjustments::modify_call): New parameter killed_ssas. > >> Instead of substitutin uses, invoke purge_transitive_uses. If > >> hash of killed SSAs has not been provided, create a temporary one > >> and release SSAs that have been added to it. > >> * tree-inline.cc (redirect_all_calls): Create > >> id->killed_new_ssa_names earlier, pass it to edge redirection, > >> adjust a comment. > >> (copy_body): Release SSAs in id->killed_new_ssa_names. > >> > >> gcc/testsuite/ChangeLog: > >> > >> 2023-05-11 Martin Jambor <mjambor@suse.cz> > >> > >> PR ipa/108007 > >> * gcc.dg/ipa/pr108007.c: New test. > >> --- > >> gcc/cgraph.cc | 10 +++- > >> gcc/cgraph.h | 9 ++- > >> gcc/ipa-param-manipulation.cc | 85 +++++++++++++++++++++-------- > >> gcc/ipa-param-manipulation.h | 3 +- > >> gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++++++++++ > >> gcc/tree-inline.cc | 28 ++++++---- > >> 6 files changed, 129 insertions(+), 38 deletions(-) > >> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c > >> > >> +/* Remove all statements that use NAME and transitively those that use the > >> + result of such statements. KILLED_SSAS contains the SSA_NAMEs that are > >> + already being or have been processed and new ones need to be added to it. > >> + The funtction only has to process situations handled by > >> + ssa_name_only_returned_p in ipa-sra.cc with the exception that it can assume > >> + it must never reach a use in a return statement. */ > >> + > >> +static void > >> +purge_transitive_uses (tree name, hash_set <tree> *killed_ssas) > >> +{ > >> + imm_use_iterator imm_iter; > >> + gimple *stmt; > >> + > >> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) > >> + { > >> + if (gimple_debug_bind_p (stmt)) > >> + { > >> + /* When runing within tree-inline, we will never end up here but > >> + adding the SSAs to killed_ssas will do the trick in this case and > >> + the respective debug statements will get reset. */ > >> + > >> + gimple_debug_bind_reset_value (stmt); > >> + update_stmt (stmt); > >> + continue; > >> + } > >> + > >> + tree lhs = NULL_TREE; > >> + if (is_gimple_assign (stmt)) > >> + lhs = gimple_assign_lhs (stmt); > >> + else if (gimple_code (stmt) == GIMPLE_PHI) > >> + lhs = gimple_phi_result (stmt); > >> + gcc_assert (lhs > >> + && (TREE_CODE (lhs) == SSA_NAME) > >> + && !gimple_vdef (stmt)); > >> + > >> + if (!killed_ssas->contains (lhs)) > >> + { > >> + killed_ssas->add (lhs); > >> + purge_transitive_uses (lhs, killed_ssas); SSA graph may be deep so this may cause stack overflow, so I think we should use worklist here (it is also easy to do). OK with that change. Honza
Hello, On Mon, Sep 25 2023, Jan Hubicka wrote: [...] >> >> +static void >> >> +purge_transitive_uses (tree name, hash_set <tree> *killed_ssas) >> >> +{ >> >> + imm_use_iterator imm_iter; >> >> + gimple *stmt; >> >> + >> >> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) >> >> + { >> >> + if (gimple_debug_bind_p (stmt)) >> >> + { >> >> + /* When runing within tree-inline, we will never end up here but >> >> + adding the SSAs to killed_ssas will do the trick in this case and >> >> + the respective debug statements will get reset. */ >> >> + >> >> + gimple_debug_bind_reset_value (stmt); >> >> + update_stmt (stmt); >> >> + continue; >> >> + } >> >> + >> >> + tree lhs = NULL_TREE; >> >> + if (is_gimple_assign (stmt)) >> >> + lhs = gimple_assign_lhs (stmt); >> >> + else if (gimple_code (stmt) == GIMPLE_PHI) >> >> + lhs = gimple_phi_result (stmt); >> >> + gcc_assert (lhs >> >> + && (TREE_CODE (lhs) == SSA_NAME) >> >> + && !gimple_vdef (stmt)); >> >> + >> >> + if (!killed_ssas->contains (lhs)) >> >> + { >> >> + killed_ssas->add (lhs); >> >> + purge_transitive_uses (lhs, killed_ssas); > > SSA graph may be deep so this may cause stack overflow, so I think we > should use worklist here (it is also easy to do). > > OK with that change. > Honza I have just committed the following after a bootstrap and testing on x86_64-linux. Thanks, Martin PR 108007 is another manifestation where we rely on DCE to clean-up after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA can leave behind statements which are fed uninitialized values and trap, even though their results are themselves never used. I have already fixed this for unused parameters in callees, this bug shows that almost the same thing can happen for removed returns, on the side of callers. This means that the issue has to be fixed elsewhere, in call redirection. This patch adds a function which looks for (and through, using a work-list) uses of operations fed specific SSA names and removes them all. That would have been easy if it wasn't for debug statements during tree-inline (from which call redirection is also invoked). Debug statements are decoupled from the rest at this point and iterating over uses of SSAs does not bring them up. During tree-inline they are handled especially at the end, I assume in order to make sure that relative ordering of UIDs are the same with and without debug info. This means that during tree-inline we need to make a hash of killed SSAs, that we already have in copy_body_data, available to the function making the purging. So the patch duly does also that, making the interface slightly ugly. gcc/ChangeLog: 2023-09-27 Martin Jambor <mjambor@suse.cz> PR ipa/108007 * cgraph.h (cgraph_edge): Add a parameter to redirect_call_stmt_to_callee. * ipa-param-manipulation.h (ipa_param_adjustments): Add a parameter to modify_call. * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New parameter killed_ssas, pass it to padjs->modify_call. * ipa-param-manipulation.cc (purge_transitive_uses): New function. (ipa_param_adjustments::modify_call): New parameter killed_ssas. Instead of substituting uses, invoke purge_transitive_uses. If hash of killed SSAs has not been provided, create a temporary one and release SSAs that have been added to it. * tree-inline.cc (redirect_all_calls): Create id->killed_new_ssa_names earlier, pass it to edge redirection, adjust a comment. (copy_body): Release SSAs in id->killed_new_ssa_names. gcc/testsuite/ChangeLog: 2023-05-11 Martin Jambor <mjambor@suse.cz> PR ipa/108007 * gcc.dg/ipa/pr108007.c: New test. --- gcc/cgraph.cc | 10 +++- gcc/cgraph.h | 9 ++- gcc/ipa-param-manipulation.cc | 88 +++++++++++++++++++++-------- gcc/ipa-param-manipulation.h | 3 +- gcc/testsuite/gcc.dg/ipa/pr108007.c | 32 +++++++++++ gcc/tree-inline.cc | 28 +++++---- 6 files changed, 132 insertions(+), 38 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108007.c diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc index e41e5ad3ae7..b82367ac342 100644 --- a/gcc/cgraph.cc +++ b/gcc/cgraph.cc @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n) speculative indirect call, remove "speculative" of the indirect call and also redirect stmt to it's final direct target. + When called from within tree-inline, KILLED_SSAs has to contain the pointer + to killed_new_ssa_names within the copy_body_data structure and SSAs + discovered to be useless (if LHS is removed) will be added to it, otherwise + it needs to be NULL. + It is up to caller to iteratively transform each "speculative" direct call as appropriate. */ gimple * -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e, + hash_set <tree> *killed_ssas) { tree decl = gimple_call_fndecl (e->call_stmt); gcall *new_stmt; @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) remove_stmt_from_eh_lp (e->call_stmt); tree old_fntype = gimple_call_fntype (e->call_stmt); - new_stmt = padjs->modify_call (e, false); + new_stmt = padjs->modify_call (e, false, killed_ssas); cgraph_node *origin = e->callee; while (origin->clone_of) origin = origin->clone_of; diff --git a/gcc/cgraph.h b/gcc/cgraph.h index cedaaac3a45..d7162efeeb4 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -1833,9 +1833,16 @@ public: speculative indirect call, remove "speculative" of the indirect call and also redirect stmt to it's final direct target. + When called from within tree-inline, KILLED_SSAs has to contain the + pointer to killed_new_ssa_names within the copy_body_data structure and + SSAs discovered to be useless (if LHS is removed) will be added to it, + otherwise it needs to be NULL. + It is up to caller to iteratively transform each "speculative" direct call as appropriate. */ - static gimple *redirect_call_stmt_to_callee (cgraph_edge *e); + static gimple *redirect_call_stmt_to_callee (cgraph_edge *e, + hash_set <tree> + *killed_ssas = nullptr); /* Create clone of edge in the node N represented by CALL_EXPR the callgraph. */ diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc index 4a185ddbdf4..0c49267ccf8 100644 --- a/gcc/ipa-param-manipulation.cc +++ b/gcc/ipa-param-manipulation.cc @@ -593,14 +593,66 @@ isra_get_ref_base_and_offset (tree expr, tree *base_p, unsigned *unit_offset_p) return true; } +/* Remove all statements that use NAME and transitively those that use the + result of such statements. KILLED_SSAS contains the SSA_NAMEs that are + already being or have been processed and new ones need to be added to it. + The funtction only has to process situations handled by + ssa_name_only_returned_p in ipa-sra.cc with the exception that it can assume + it must never reach a use in a return statement. */ + +static void +purge_transitive_uses (tree name, hash_set <tree> *killed_ssas) +{ + imm_use_iterator imm_iter; + gimple *stmt; + auto_vec <tree, 4> worklist; + + worklist.safe_push (name); + while (!worklist.is_empty ()) + { + tree cur_name = worklist.pop (); + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, cur_name) + { + if (gimple_debug_bind_p (stmt)) + { + /* When runing within tree-inline, we will never end up here but + adding the SSAs to killed_ssas will do the trick in this case + and the respective debug statements will get reset. */ + gimple_debug_bind_reset_value (stmt); + update_stmt (stmt); + continue; + } + + tree lhs = NULL_TREE; + if (is_gimple_assign (stmt)) + lhs = gimple_assign_lhs (stmt); + else if (gimple_code (stmt) == GIMPLE_PHI) + lhs = gimple_phi_result (stmt); + gcc_assert (lhs + && (TREE_CODE (lhs) == SSA_NAME) + && !gimple_vdef (stmt)); + if (!killed_ssas->add (lhs)) + { + worklist.safe_push (lhs); + gimple_stmt_iterator gsi = gsi_for_stmt (stmt); + gsi_remove (&gsi, true); + } + } + } +} + /* Modify actual arguments of a function call in statement currently belonging to CS, and make it call CS->callee->decl. Return the new statement that replaced the old one. When invoked, cfun and current_function_decl have to - be set to the caller. */ + be set to the caller. When called from within tree-inline, KILLED_SSAs has + to contain the pointer to killed_new_ssa_names within the copy_body_data + structure and SSAs discovered to be useless (if LHS is removed) will be + added to it, otherwise it needs to be NULL. */ gcall * ipa_param_adjustments::modify_call (cgraph_edge *cs, - bool update_references) + bool update_references, + hash_set <tree> *killed_ssas) { gcall *stmt = cs->call_stmt; tree callee_decl = cs->callee->decl; @@ -910,32 +962,20 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, gcall *new_stmt = gimple_build_call_vec (callee_decl, vargs); - tree ssa_to_remove = NULL; + hash_set <tree> *ssas_to_remove = NULL; if (tree lhs = gimple_call_lhs (stmt)) { if (!m_skip_return) gimple_call_set_lhs (new_stmt, lhs); else if (TREE_CODE (lhs) == SSA_NAME) { - /* LHS should now by a default-def SSA. Unfortunately default-def - SSA_NAMEs need a backing variable (or at least some code examining - SSAs assumes it is non-NULL). So we either have to re-use the - decl we have at hand or introdice a new one. */ - tree repl = create_tmp_var (TREE_TYPE (lhs), "removed_return"); - repl = get_or_create_ssa_default_def (cfun, repl); - SSA_NAME_IS_DEFAULT_DEF (repl) = true; - imm_use_iterator ui; - use_operand_p use_p; - gimple *using_stmt; - FOR_EACH_IMM_USE_STMT (using_stmt, ui, lhs) + if (!killed_ssas) { - FOR_EACH_IMM_USE_ON_STMT (use_p, ui) - { - SET_USE (use_p, repl); - } - update_stmt (using_stmt); + ssas_to_remove = new hash_set<tree> (8); + killed_ssas = ssas_to_remove; } - ssa_to_remove = lhs; + killed_ssas->add (lhs); + purge_transitive_uses (lhs, killed_ssas); } } @@ -954,8 +994,12 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, fprintf (dump_file, "\n"); } gsi_replace (&gsi, new_stmt, true); - if (ssa_to_remove) - release_ssa_name (ssa_to_remove); + if (ssas_to_remove) + { + for (tree sn : *ssas_to_remove) + release_ssa_name (sn); + delete ssas_to_remove; + } if (update_references) do { diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h index d6a26e9ad36..b7e56fe6379 100644 --- a/gcc/ipa-param-manipulation.h +++ b/gcc/ipa-param-manipulation.h @@ -224,7 +224,8 @@ public: /* Modify a call statement arguments (and possibly remove the return value) as described in the data fields of this class. */ - gcall *modify_call (cgraph_edge *cs, bool update_references); + gcall *modify_call (cgraph_edge *cs, bool update_references, + hash_set <tree> *killed_ssas); /* Return if the first parameter is left intact. */ bool first_param_intact_p (); /* Build a function type corresponding to the modified call. */ diff --git a/gcc/testsuite/gcc.dg/ipa/pr108007.c b/gcc/testsuite/gcc.dg/ipa/pr108007.c new file mode 100644 index 00000000000..77fc95975cf --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr108007.c @@ -0,0 +1,32 @@ +/* { dg-do run } */ +/* { dg-options "-Os -fno-dce -fno-tree-dce -g" } */ + +/* This tests that when IPA-SRA removes a LHS of a call statement which, in the + original source, is fed into a useless operation which however can trap when + given nonsensical input, that we remove it even when the user has turned off + normal DCE. */ + +int a, b, d, e, f = 10000000, h; +short c, g; +static int *i() { + g = f; + L: + h = e = ~g; + g = ~f % g & e; + if (!g) + goto L; + c++; + while (g < 1) + ; + return &a; +} +static void k() { + int *l, m = 2; + l = i(); + for (; d < 1; d++) + m |= *l >= b; +} +int main() { + k(); + return 0; +} diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index d63060c9429..8daef6955fd 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -2988,20 +2988,19 @@ redirect_all_calls (copy_body_data * id, basic_block bb) struct cgraph_edge *edge = id->dst_node->get_edge (stmt); if (edge) { + if (!id->killed_new_ssa_names) + id->killed_new_ssa_names = new hash_set<tree> (16); gimple *new_stmt - = cgraph_edge::redirect_call_stmt_to_callee (edge); - /* If IPA-SRA transformation, run as part of edge redirection, - removed the LHS because it is unused, save it to - killed_new_ssa_names so that we can prune it from debug - statements. */ + = cgraph_edge::redirect_call_stmt_to_callee (edge, + id->killed_new_ssa_names); if (old_lhs && TREE_CODE (old_lhs) == SSA_NAME && !gimple_call_lhs (new_stmt)) - { - if (!id->killed_new_ssa_names) - id->killed_new_ssa_names = new hash_set<tree> (16); - id->killed_new_ssa_names->add (old_lhs); - } + /* In case of IPA-SRA removing the LHS, the name should have + been already added to the hash. But in case of redirecting + to builtin_unreachable it was not and the name still should + be pruned from debug statements. */ + id->killed_new_ssa_names->add (old_lhs); if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) gimple_purge_dead_eh_edges (bb); @@ -3328,8 +3327,13 @@ copy_body (copy_body_data *id, body = copy_cfg_body (id, entry_block_map, exit_block_map, new_entry); copy_debug_stmts (id); - delete id->killed_new_ssa_names; - id->killed_new_ssa_names = NULL; + if (id->killed_new_ssa_names) + { + for (tree sn : *id->killed_new_ssa_names) + release_ssa_name (sn); + delete id->killed_new_ssa_names; + id->killed_new_ssa_names = NULL; + } return body; }
On Tue, 3 Oct 2023, Martin Jambor wrote: > > SSA graph may be deep so this may cause stack overflow, so I think we > > should use worklist here (it is also easy to do). > > > > OK with that change. > > Honza > > I have just committed the following after a bootstrap and testing on > x86_64-linux. This has regressed the native `powerpc64le-linux-gnu' configuration, which doesn't bootstrap here anymore: Comparing stages 2 and 3 Bootstrap comparison failure! powerpc64le-linux-gnu/libstdc++-v3/src/compatibility-ldbl.o differs powerpc64le-linux-gnu/libstdc++-v3/src/.libs/compatibility-ldbl.o differs I have double-checked this is indeed the offending commit, the compiler bootstraps just fine as at commit 7eb5ce7f58ed ("Remove pass counting in VRP."). Shall I file a PR, or can you handle it regardless? Let me know if you need anything from me. Maciej
On Wed, Oct 4, 2023 at 5:08 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote: > > On Tue, 3 Oct 2023, Martin Jambor wrote: > > > > SSA graph may be deep so this may cause stack overflow, so I think we > > > should use worklist here (it is also easy to do). > > > > > > OK with that change. > > > Honza > > > > I have just committed the following after a bootstrap and testing on > > x86_64-linux. > > This has regressed the native `powerpc64le-linux-gnu' configuration, > which doesn't bootstrap here anymore: > > Comparing stages 2 and 3 > Bootstrap comparison failure! > powerpc64le-linux-gnu/libstdc++-v3/src/compatibility-ldbl.o differs > powerpc64le-linux-gnu/libstdc++-v3/src/.libs/compatibility-ldbl.o differs > > I have double-checked this is indeed the offending commit, the compiler > bootstraps just fine as at commit 7eb5ce7f58ed ("Remove pass counting in > VRP."). > > Shall I file a PR, or can you handle it regardless? Let me know if you > need anything from me. It is already filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111688 . Thanks, Andrew > > Maciej
diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc index e8f9bec8227..5e923bf0557 100644 --- a/gcc/cgraph.cc +++ b/gcc/cgraph.cc @@ -1403,11 +1403,17 @@ cgraph_edge::redirect_callee (cgraph_node *n) speculative indirect call, remove "speculative" of the indirect call and also redirect stmt to it's final direct target. + When called from within tree-inline, KILLED_SSAs has to contain the pointer + to killed_new_ssa_names within the copy_body_data structure and SSAs + discovered to be useless (if LHS is removed) will be added to it, otherwise + it needs to be NULL. + It is up to caller to iteratively transform each "speculative" direct call as appropriate. */ gimple * -cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) +cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e, + hash_set <tree> *killed_ssas) { tree decl = gimple_call_fndecl (e->call_stmt); gcall *new_stmt; @@ -1527,7 +1533,7 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) remove_stmt_from_eh_lp (e->call_stmt); tree old_fntype = gimple_call_fntype (e->call_stmt); - new_stmt = padjs->modify_call (e, false); + new_stmt = padjs->modify_call (e, false, killed_ssas); cgraph_node *origin = e->callee; while (origin->clone_of) origin = origin->clone_of; diff --git a/gcc/cgraph.h b/gcc/cgraph.h index f5f54769eda..c1a3691b6f5 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -1833,9 +1833,16 @@ public: speculative indirect call, remove "speculative" of the indirect call and also redirect stmt to it's final direct target. + When called from within tree-inline, KILLED_SSAs has to contain the + pointer to killed_new_ssa_names within the copy_body_data structure and + SSAs discovered to be useless (if LHS is removed) will be added to it, + otherwise it needs to be NULL. + It is up to caller to iteratively transform each "speculative" direct call as appropriate. */ - static gimple *redirect_call_stmt_to_callee (cgraph_edge *e); + static gimple *redirect_call_stmt_to_callee (cgraph_edge *e, + hash_set <tree> + *killed_ssas = nullptr); /* Create clone of edge in the node N represented by CALL_EXPR the callgraph. */ diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc index a286af7f5d9..bf2adeb4bd6 100644 --- a/gcc/ipa-param-manipulation.cc +++ b/gcc/ipa-param-manipulation.cc @@ -593,14 +593,63 @@ isra_get_ref_base_and_offset (tree expr, tree *base_p, unsigned *unit_offset_p) return true; } +/* Remove all statements that use NAME and transitively those that use the + result of such statements. KILLED_SSAS contains the SSA_NAMEs that are + already being or have been processed and new ones need to be added to it. + The funtction only has to process situations handled by + ssa_name_only_returned_p in ipa-sra.cc with the exception that it can assume + it must never reach a use in a return statement. */ + +static void +purge_transitive_uses (tree name, hash_set <tree> *killed_ssas) +{ + imm_use_iterator imm_iter; + gimple *stmt; + + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) + { + if (gimple_debug_bind_p (stmt)) + { + /* When runing within tree-inline, we will never end up here but + adding the SSAs to killed_ssas will do the trick in this case and + the respective debug statements will get reset. */ + + gimple_debug_bind_reset_value (stmt); + update_stmt (stmt); + continue; + } + + tree lhs = NULL_TREE; + if (is_gimple_assign (stmt)) + lhs = gimple_assign_lhs (stmt); + else if (gimple_code (stmt) == GIMPLE_PHI) + lhs = gimple_phi_result (stmt); + gcc_assert (lhs + && (TREE_CODE (lhs) == SSA_NAME) + && !gimple_vdef (stmt)); + + if (!killed_ssas->contains (lhs)) + { + killed_ssas->add (lhs); + purge_transitive_uses (lhs, killed_ssas); + gimple_stmt_iterator gsi = gsi_for_stmt (stmt); + gsi_remove (&gsi, true); + } + } +} + /* Modify actual arguments of a function call in statement currently belonging to CS, and make it call CS->callee->decl. Return the new statement that replaced the old one. When invoked, cfun and current_function_decl have to - be set to the caller. */ + be set to the caller. When called from within tree-inline, KILLED_SSAs has + to contain the pointer to killed_new_ssa_names within the copy_body_data + structure and SSAs discovered to be useless (if LHS is removed) will be + added to it, otherwise it needs to be NULL. */ gcall * ipa_param_adjustments::modify_call (cgraph_edge *cs, - bool update_references) + bool update_references, + hash_set <tree> *killed_ssas) { gcall *stmt = cs->call_stmt; tree callee_decl = cs->callee->decl; @@ -910,32 +959,20 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, gcall *new_stmt = gimple_build_call_vec (callee_decl, vargs); - tree ssa_to_remove = NULL; + hash_set <tree> *ssas_to_remove = NULL; if (tree lhs = gimple_call_lhs (stmt)) { if (!m_skip_return) gimple_call_set_lhs (new_stmt, lhs); else if (TREE_CODE (lhs) == SSA_NAME) { - /* LHS should now by a default-def SSA. Unfortunately default-def - SSA_NAMEs need a backing variable (or at least some code examining - SSAs assumes it is non-NULL). So we either have to re-use the - decl we have at hand or introdice a new one. */ - tree repl = create_tmp_var (TREE_TYPE (lhs), "removed_return"); - repl = get_or_create_ssa_default_def (cfun, repl); - SSA_NAME_IS_DEFAULT_DEF (repl) = true; - imm_use_iterator ui; - use_operand_p use_p; - gimple *using_stmt; - FOR_EACH_IMM_USE_STMT (using_stmt, ui, lhs) + if (!killed_ssas) { - FOR_EACH_IMM_USE_ON_STMT (use_p, ui) - { - SET_USE (use_p, repl); - } - update_stmt (using_stmt); + ssas_to_remove = new hash_set<tree> (8); + killed_ssas = ssas_to_remove; } - ssa_to_remove = lhs; + killed_ssas->add (lhs); + purge_transitive_uses (lhs, killed_ssas); } } @@ -954,8 +991,12 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs, fprintf (dump_file, "\n"); } gsi_replace (&gsi, new_stmt, true); - if (ssa_to_remove) - release_ssa_name (ssa_to_remove); + if (ssas_to_remove) + { + for (tree sn : *ssas_to_remove) + release_ssa_name (sn); + delete ssas_to_remove; + } if (update_references) do { diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h index 9798eedf0b6..5b2f90f8307 100644 --- a/gcc/ipa-param-manipulation.h +++ b/gcc/ipa-param-manipulation.h @@ -224,7 +224,8 @@ public: /* Modify a call statement arguments (and possibly remove the return value) as described in the data fields of this class. */ - gcall *modify_call (cgraph_edge *cs, bool update_references); + gcall *modify_call (cgraph_edge *cs, bool update_references, + hash_set <tree> *killed_ssas); /* Return if the first parameter is left intact. */ bool first_param_intact_p (); /* Build a function type corresponding to the modified call. */ diff --git a/gcc/testsuite/gcc.dg/ipa/pr108007.c b/gcc/testsuite/gcc.dg/ipa/pr108007.c new file mode 100644 index 00000000000..77fc95975cf --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr108007.c @@ -0,0 +1,32 @@ +/* { dg-do run } */ +/* { dg-options "-Os -fno-dce -fno-tree-dce -g" } */ + +/* This tests that when IPA-SRA removes a LHS of a call statement which, in the + original source, is fed into a useless operation which however can trap when + given nonsensical input, that we remove it even when the user has turned off + normal DCE. */ + +int a, b, d, e, f = 10000000, h; +short c, g; +static int *i() { + g = f; + L: + h = e = ~g; + g = ~f % g & e; + if (!g) + goto L; + c++; + while (g < 1) + ; + return &a; +} +static void k() { + int *l, m = 2; + l = i(); + for (; d < 1; d++) + m |= *l >= b; +} +int main() { + k(); + return 0; +} diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index 63a19f8d1d8..1c20e9545d1 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -2982,20 +2982,19 @@ redirect_all_calls (copy_body_data * id, basic_block bb) struct cgraph_edge *edge = id->dst_node->get_edge (stmt); if (edge) { + if (!id->killed_new_ssa_names) + id->killed_new_ssa_names = new hash_set<tree> (16); gimple *new_stmt - = cgraph_edge::redirect_call_stmt_to_callee (edge); - /* If IPA-SRA transformation, run as part of edge redirection, - removed the LHS because it is unused, save it to - killed_new_ssa_names so that we can prune it from debug - statements. */ + = cgraph_edge::redirect_call_stmt_to_callee (edge, + id->killed_new_ssa_names); if (old_lhs && TREE_CODE (old_lhs) == SSA_NAME && !gimple_call_lhs (new_stmt)) - { - if (!id->killed_new_ssa_names) - id->killed_new_ssa_names = new hash_set<tree> (16); - id->killed_new_ssa_names->add (old_lhs); - } + /* In case of IPA-SRA removing the LHS, the name should have + been already added to the hash. But in case of redirecting + to builtin_unreachable it was not and the name still should + be pruned from debug statements. */ + id->killed_new_ssa_names->add (old_lhs); if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) gimple_purge_dead_eh_edges (bb); @@ -3322,8 +3321,13 @@ copy_body (copy_body_data *id, body = copy_cfg_body (id, entry_block_map, exit_block_map, new_entry); copy_debug_stmts (id); - delete id->killed_new_ssa_names; - id->killed_new_ssa_names = NULL; + if (id->killed_new_ssa_names) + { + for (tree sn : *id->killed_new_ssa_names) + release_ssa_name (sn); + delete id->killed_new_ssa_names; + id->killed_new_ssa_names = NULL; + } return body; }