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 <mjam...@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 <mjam...@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
