On Thu, 28 May 2020, Martin Jambor wrote:
> PR 93385 reveals that if the user explicitely disables DCE, IPA-SRA
> can leave behind statements which are useless because their results
> are eventually not used but can have problematic side effects,
> especially since their inputs are now bogus that useless parameters
> were removed.
>
> This patch fixes the problem by doing a similar def-use walk when
> materializing clones, marking which statements should not be copied
> and which SSA_NAMEs will be removed by call redirections and now need
> to be replaced with anything valid. Default-definition SSA_NAMEs of
> parameters which are removed and all SSA_NAMEs derived from them (in a
> phi node or a simple assignment statement) are then remapped to
> error_mark_node - a sure way to spot it if any is left in place.
>
> There is one exception to the above rule, if such SSA_NAMEs appear as
> an argument of a call, they need to be removed by call redirection and
> not as part of clone materialization. So to have something valid
> there until that time, this patch pulls out dummy declarations out of
> thin air. If you do not like that, see patch number 4 in the series,
> which changes this, but probably in a controversial way.
>
> This patch only resets debug statements using the removed SSA_NAMEs.
> The first follow-up patch adjusts debug statements in the current
> function to still try to make the removed values available in debugger
> in the current function and the subsequent one also in other functions
> where they are passed.
>
> gcc/ChangeLog:
>
> 2020-05-14 Martin Jambor <[email protected]>
>
> PR ipa/93385
> * ipa-param-manipulation.h (class ipa_param_body_adjustments): New
> members m_dead_stmts, m_dead_ssas, mark_dead_statements and
> get_removed_call_arg_placeholder.
> * ipa-param-manipulation.c (phi_arg_will_live_p): New function.
> (ipa_param_body_adjustments::mark_dead_statements): New method.
> (ipa_param_body_adjustments::common_initialization): Call it.
> (ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize
> new mwmbers.
> (ipa_param_body_adjustments::get_removed_call_arg_placeholder): New.
> (ipa_param_body_adjustments::modify_call_stmt): Replace dead SSAs
> with dummy decls.
> * tree-inline.c (remap_gimple_stmt): Do not copy dead statements,
> reset dead debug statements.
> (copy_phis_for_bb): Do not copy dead PHI nodes.
>
> gcc/testsuite/ChangeLog:
>
> 2020-05-14 Martin Jambor <[email protected]>
>
> PR ipa/93385
> * gcc.dg/ipa/pr93385.c: New test.
> * gcc.dg/ipa/ipa-sra-23.c: Likewise.
> ---
> gcc/ipa-param-manipulation.c | 142 ++++++++++++++++++++++++--
> gcc/ipa-param-manipulation.h | 8 ++
> gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c | 24 +++++
> gcc/testsuite/gcc.dg/ipa/pr93385.c | 27 +++++
> gcc/tree-inline.c | 18 +++-
> 5 files changed, 205 insertions(+), 14 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93385.c
>
> diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
> index 978916057f0..1f47f3a4268 100644
> --- a/gcc/ipa-param-manipulation.c
> +++ b/gcc/ipa-param-manipulation.c
> @@ -953,6 +953,99 @@ ipa_param_body_adjustments::carry_over_param (tree t)
> return new_parm;
> }
>
> +/* Return true if BLOCKS_TO_COPY is NULL or if PHI has an argument ARG in
> + position that corresponds to an edge that is coming from a block that has
> + the corresponding bit set in BLOCKS_TO_COPY. */
> +
> +static bool
> +phi_arg_will_live_p (gphi *phi, bitmap blocks_to_copy, tree arg)
> +{
> + bool arg_will_survive = false;
> + if (!blocks_to_copy)
> + arg_will_survive = true;
> + else
> + for (unsigned i = 0; i < gimple_phi_num_args (phi); i++)
> + if (gimple_phi_arg_def (phi, i) == arg
> + && bitmap_bit_p (blocks_to_copy,
> + gimple_phi_arg_edge (phi, i)->src->index))
> + {
> + arg_will_survive = true;
> + break;
> + }
> + return arg_will_survive;
> +}
> +
> +/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
> + any replacement or splitting. */
> +
> +void
> +ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
> +{
> + if (!is_gimple_reg (dead_param))
Hmm, so non-registers are not a problem? I guess IPA SRA simply
ensures there are no actual uses (but call arguments) in that case?
Please clearly document this function matches IPA SRA capabilities.
> + return;
> + tree parm_ddef = ssa_default_def (m_id->src_cfun, dead_param);
> + if (!parm_ddef || has_zero_uses (parm_ddef))
> + return;
> +
> + auto_vec<tree, 4> stack;
> + m_dead_ssas.add (parm_ddef);
> + stack.safe_push (parm_ddef);
> + while (!stack.is_empty ())
> + {
> + tree t = stack.pop ();
> +
> + imm_use_iterator imm_iter;
> + gimple *stmt;
> +
> + insert_decl_map (m_id, t, error_mark_node);
> + FOR_EACH_IMM_USE_STMT (stmt, imm_iter, t)
> + {
> + if (is_gimple_call (stmt)
> + || (m_id->blocks_to_copy
> + && !bitmap_bit_p (m_id->blocks_to_copy,
> + gimple_bb (stmt)->index)))
> + continue;
> +
> + if (is_gimple_debug (stmt))
> + {
> + m_dead_stmts.add (stmt);
> + gcc_assert (gimple_debug_bind_p (stmt));
> + }
> + else if (gimple_code (stmt) == GIMPLE_PHI)
> + {
> + gphi *phi = as_a <gphi *> (stmt);
> + if (phi_arg_will_live_p (phi, m_id->blocks_to_copy, t))
> + {
> + m_dead_stmts.add (phi);
> + tree res = gimple_phi_result (phi);
> + if (!m_dead_ssas.contains (res))
You can combine this with the add below which returns false if the
item was not already present.
> + {
> + stack.safe_push (res);
> + m_dead_ssas.add (res);
> + }
> + }
> + }
> + else if (is_gimple_assign (stmt))
> + {
> + m_dead_stmts.add (stmt);
> + if (!gimple_clobber_p (stmt))
> + {
> + tree lhs = gimple_assign_lhs (stmt);
> + gcc_assert (TREE_CODE (lhs) == SSA_NAME);
> + if (!m_dead_ssas.contains (lhs))
> + {
> + m_dead_ssas.add (lhs);
> + stack.safe_push (lhs);
> + }
> + }
> + }
> + else
> + /* IPA-SRA does not analyze other types of statements. */
> + gcc_unreachable ();
> + }
> + }
> +}
> +
> /* Common initialization performed by all ipa_param_body_adjustments
> constructors. OLD_FNDECL is the declaration we take original arguments
> from, (it may be the same as M_FNDECL). VARS, if non-NULL, is a pointer
> to
> @@ -1095,6 +1188,11 @@ ipa_param_body_adjustments::common_initialization
> (tree old_fndecl,
> /* Declare this new variable. */
> DECL_CHAIN (var) = *vars;
> *vars = var;
> +
> + /* If this is not a split but a real removal, init hash sets
> + that will guide what not to copy to the new body. */
> + if (!isra_dummy_decls[i])
> + mark_dead_statements (m_oparms[i]);
> }
> }
> else
> @@ -1151,9 +1249,10 @@ ipa_param_body_adjustments
> ::ipa_param_body_adjustments (vec<ipa_adjusted_param, va_gc> *adj_params,
> tree fndecl)
> : m_adj_params (adj_params), m_adjustments (NULL), m_reset_debug_decls (),
> - m_split_modifications_p (false), m_fndecl (fndecl), m_id (NULL),
> - m_oparms (), m_new_decls (), m_new_types (), m_replacements (),
> - m_removed_decls (), m_removed_map (), m_method2func (false)
> + m_split_modifications_p (false), m_dead_stmts (), m_dead_ssas (),
> + m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (),
> + m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
> + m_method2func (false)
> {
> common_initialization (fndecl, NULL, NULL);
> }
> @@ -1167,9 +1266,9 @@ ipa_param_body_adjustments
> ::ipa_param_body_adjustments (ipa_param_adjustments *adjustments,
> tree fndecl)
> : m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
> - m_reset_debug_decls (), m_split_modifications_p (false), m_fndecl
> (fndecl),
> - m_id (NULL), m_oparms (), m_new_decls (), m_new_types (),
> - m_replacements (), m_removed_decls (), m_removed_map (),
> + m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
> + m_dead_ssas (), m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls
> (),
> + m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
> m_method2func (false)
> {
> common_initialization (fndecl, NULL, NULL);
> @@ -1190,9 +1289,10 @@ ipa_param_body_adjustments
> copy_body_data *id, tree *vars,
> vec<ipa_replace_map *, va_gc> *tree_map)
> : m_adj_params (adjustments->m_adj_params), m_adjustments (adjustments),
> - m_reset_debug_decls (), m_split_modifications_p (false), m_fndecl
> (fndecl),
> - m_id (id), m_oparms (), m_new_decls (), m_new_types (), m_replacements
> (),
> - m_removed_decls (), m_removed_map (), m_method2func (false)
> + m_reset_debug_decls (), m_split_modifications_p (false), m_dead_stmts (),
> + m_dead_ssas (),m_fndecl (fndecl), m_id (id), m_oparms (), m_new_decls (),
> + m_new_types (), m_replacements (), m_removed_decls (), m_removed_map (),
> + m_method2func (false)
> {
> common_initialization (old_fndecl, vars, tree_map);
> }
> @@ -1519,6 +1619,19 @@ remap_split_decl_to_dummy (tree *tp, int
> *walk_subtrees, void *data)
> return NULL_TREE;
> }
>
> +/* Given ARG which is a SSA_NAME call argument which we are however removing
> + from the current function and which will be thus removed from the call
> + statement by ipa_param_adjustments::modify_call, return something that can
> + be used as a placeholder and which the operand scanner will accept until
> + then. */
> +
> +tree
> +ipa_param_body_adjustments::get_removed_call_arg_placeholder (tree arg)
> +{
> + tree t = create_tmp_var (TREE_TYPE (arg));
If it is temporarily then use _raw. It looks like you can get called
multiple times for the same arg and each time you get a new temporary
decl? Ugh.
> + insert_decl_map (m_id, t, t);
I wonder why you can't use/keep the SSA default def of the removed
parameter instead? Or a literal zero? That is, below you don't
seem to be anything special during inlining itself so I presume
those are all removed by call redirection? Why are the actual
parameters then still needed? (and error_mark_node doesn't work?)
> + return t;
> +}
>
> /* If the call statement pointed at by STMT_P contains any expressions that
> need to replaced with a different one as noted by ADJUSTMENTS, do so. f
> the
> @@ -1658,7 +1771,10 @@ ipa_param_body_adjustments::modify_call_stmt (gcall
> **stmt_p)
> else
> {
> tree t = gimple_call_arg (stmt, i);
> - modify_expression (&t, true);
> + if (TREE_CODE (t) == SSA_NAME && m_dead_ssas.contains(t))
space after function call (likewise elsewhere).
> + t = get_removed_call_arg_placeholder (t);
> + else
> + modify_expression (&t, true);
> vargs.safe_push (t);
> }
> }
> @@ -1680,7 +1796,11 @@ ipa_param_body_adjustments::modify_call_stmt (gcall
> **stmt_p)
> for (unsigned i = 0; i < nargs; i++)
> {
> tree *t = gimple_call_arg_ptr (stmt, i);
> - modified |= modify_expression (t, true);
> +
> + if (TREE_CODE (*t) == SSA_NAME && m_dead_ssas.contains(*t))
> + *t = get_removed_call_arg_placeholder (*t);
> + else
> + modified |= modify_expression (t, true);
> }
>
> if (gimple_call_lhs (stmt))
> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
> index 0b038ea57f1..59060ae5dcc 100644
> --- a/gcc/ipa-param-manipulation.h
> +++ b/gcc/ipa-param-manipulation.h
> @@ -370,6 +370,12 @@ public:
> /* Set to true if there are any IPA_PARAM_OP_SPLIT adjustments among stored
> adjustments. */
> bool m_split_modifications_p;
> +
> + /* Sets of statements and SSA_NAMEs that only manipulate data from
> parameters
> + removed because they are not necessary. */
> + hash_set<gimple *> m_dead_stmts;
> + hash_set<tree> m_dead_ssas;
> +
> private:
> void common_initialization (tree old_fndecl, tree *vars,
> vec<ipa_replace_map *, va_gc> *tree_map);
> @@ -383,6 +389,8 @@ private:
> bool modify_call_stmt (gcall **stmt_p);
> bool modify_cfun_body ();
> void reset_debug_stmts ();
> + void mark_dead_statements (tree dead_param);
> + tree get_removed_call_arg_placeholder (tree arg);
>
> /* Declaration of the function that is being transformed. */
>
> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
> b/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
> new file mode 100644
> index 00000000000..f438b509614
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-23.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +extern int g;
> +
> +static int __attribute__((noinline))
> +bar (int i, int j)
> +{
> + return 2*g + i;
> +}
> +
> +static int __attribute__((noinline))
> +foo (int i, int j)
> +{
> + if (i > 5)
> + j = 22;
> + return bar (i, j) + 1;
> +}
> +
> +int
> +entry (int l, int k)
> +{
> + return foo (l, k);
> +}
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr93385.c
> b/gcc/testsuite/gcc.dg/ipa/pr93385.c
> new file mode 100644
> index 00000000000..6d1d0d7cd27
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr93385.c
> @@ -0,0 +1,27 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-dce -fno-ipa-cp -fno-tree-dce" } */
> +
> +char a, b;
> +
> +#ifdef __SIZEOF_INT128__
> +#define T unsigned __int128
> +#else
> +#define T unsigned
> +#endif
> +
> +static inline int
> +c (T d)
> +{
> + char e = 0;
> + d %= (unsigned) d;
> + e -= 0;
> + __builtin_strncpy (&a, &e, 1);
> + return e + b;
> +}
> +
> +int
> +main (void)
> +{
> + c (~0);
> + return 0;
> +}
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index 3160ca3f88a..60087dd5e7b 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -1524,6 +1524,11 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
> : !opt_for_fn (id->dst_fn, flag_var_tracking_assignments)))
> return NULL;
>
> + if (!is_gimple_debug (stmt)
> + && id->param_body_adjs
> + && id->param_body_adjs->m_dead_stmts.contains (stmt))
> + return NULL;
> +
> /* Begin by recognizing trees that we'll completely rewrite for the
> inlining context. Our output for these trees is completely
> different from our input (e.g. RETURN_EXPR is deleted and morphs
> @@ -1788,10 +1793,15 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
>
> if (gimple_debug_bind_p (stmt))
> {
> + tree value;
> + if (id->param_body_adjs
> + && id->param_body_adjs->m_dead_stmts.contains (stmt))
> + value = NULL_TREE;
> + else
> + value = gimple_debug_bind_get_value (stmt);
> gdebug *copy
> = gimple_build_debug_bind (gimple_debug_bind_get_var (stmt),
> - gimple_debug_bind_get_value (stmt),
> - stmt);
> + value, stmt);
> if (id->reset_location)
> gimple_set_location (copy, input_location);
> id->debug_stmts.safe_push (copy);
> @@ -2671,7 +2681,9 @@ copy_phis_for_bb (basic_block bb, copy_body_data *id)
> phi = si.phi ();
> res = PHI_RESULT (phi);
> new_res = res;
> - if (!virtual_operand_p (res))
> + if (!virtual_operand_p (res)
> + && (!id->param_body_adjs
> + || !id->param_body_adjs->m_dead_stmts.contains (phi)))
> {
> walk_tree (&new_res, copy_tree_body_r, id, NULL);
> if (EDGE_COUNT (new_bb->preds) == 0)
>
