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 <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
