From patchwork Fri Mar 12 12:39:16 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 1451985 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4DxlkS2vLpz9sVw for ; Fri, 12 Mar 2021 23:39:28 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3D5F8398E45E; Fri, 12 Mar 2021 12:39:26 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id DC9FF386EC52 for ; Fri, 12 Mar 2021 12:39:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DC9FF386EC52 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mjambor@suse.cz X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 9B27CB0D7 for ; Fri, 12 Mar 2021 12:39:16 +0000 (UTC) From: Martin Jambor To: GCC Patches Cc: Subject: [PATCH 1/2] ipa-sra: Introduce a mini-DCE to tree-inline.c (PR 93385) User-Agent: Notmuch/0.31.3 (https://notmuchmail.org) Emacs/27.1 (x86_64-suse-linux-gnu) Date: Fri, 12 Mar 2021 13:39:16 +0100 Message-ID: MIME-Version: 1.0 X-Spam-Status: No, score=-3039.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" Hi, 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 def-use walk when materializing clones, marking which statements should not be copied and which SSA_NAMEs do not need to be computed because eventually they would be DCEd. Then tree-inline.c code simply does not copy them. See the follow-up patch for what this means for debug statements. There is one exception to the above non-copying rule: If such an SSA_NAME appears as an argument of a call, it needs to be removed by call redirection and not as part of clone materialization. So to have something valid there until that time, this patch uses the VAR_DECL that is already created to serve as a base of non-default-def SSA_NAMEs of the removed parameter. This is technically incorrect because the argument may be only causally related to the removed parameter, but it would be only there until call redirection and this avoids creating another VAR_DECL, which was the major objection to this patch in summer. I think I have managed to design a way how to avoid it and generally make the call statements a bit less clunky in between clone materialization and call redirection. It involves storing essential information in call summaries (also moving information from clone_info::performed_splits there) but the patch is big even though I have not yet finished writing it and it is not stage 4 material. So, if we want to fix the issue in GCC 11 (and 10?), I would propose this one - almost all of it is needed for the sophisticated fix too. It has passed bootstrap, LTO bootstrap and profiled LTO bootstrap and testing on x86_64-linux and normal bootstrap and testing on aarch64-linux. Alternatively, we can just mitigate all known manifestations of the issue with https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93385#c11 and only include the sophisticated fix using call summaries in GCC 12. (Or we can of course decide to do nothing for 11.1 and wait for the final patch in GCC 12.) Thanks, Martin gcc/ChangeLog: 2021-03-10 Martin Jambor PR ipa/93385 * ipa-param-manipulation.h (class ipa_param_body_adjustments): New members m_dead_stmts, m_dead_ssas, mark_dead_statements and modify_call_argument. * 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::modify_call_argument): New. (ipa_param_body_adjustments::modify_call_stmt): Use modify_call_argument. * 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 PR ipa/93385 * gcc.dg/ipa/pr93385.c: New test. * gcc.dg/ipa/ipa-sra-23.c: Likewise. --- gcc/ipa-param-manipulation.c | 144 ++++++++++++++++++++++++-- gcc/ipa-param-manipulation.h | 9 ++ 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, 208 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 132bb24f76a..f6b569a8a8b 100644 --- a/gcc/ipa-param-manipulation.c +++ b/gcc/ipa-param-manipulation.c @@ -970,6 +970,97 @@ 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. REPL is the replacement VAR_SECL to base any + remaining uses of a removed parameter on. */ + +void +ipa_param_body_adjustments::mark_dead_statements (tree dead_param, tree repl) +{ + /* Current IPA analyses which remove unused parameters never remove a + non-gimple register ones which have any use except as parameters in other + calls, so we can safely leve them as they are. */ + if (!is_gimple_reg (dead_param)) + return; + tree parm_ddef = ssa_default_def (m_id->src_cfun, dead_param); + if (!parm_ddef || has_zero_uses (parm_ddef)) + return; + + auto_vec stack; + m_dead_ssas.put (parm_ddef, repl); + 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 (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.put (res, repl)) + stack.safe_push (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.put (lhs, repl)) + 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 @@ -1113,6 +1204,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], var); } } else @@ -1169,9 +1265,10 @@ ipa_param_body_adjustments ::ipa_param_body_adjustments (vec *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); } @@ -1185,9 +1282,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); @@ -1208,9 +1305,10 @@ ipa_param_body_adjustments copy_body_data *id, tree *vars, vec *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); } @@ -1538,6 +1636,30 @@ remap_split_decl_to_dummy (tree *tp, int *walk_subtrees, void *data) } + +/* If the expression *EXPR_P, which is a call argument, should be replaced, do + so and retrn true. Otherwise return false. */ + +bool +ipa_param_body_adjustments::modify_call_argument (tree *expr_p) +{ + tree *r; + if (TREE_CODE (*expr_p) == SSA_NAME + && (r = m_dead_ssas.get (*expr_p))) + { + /* The argument is not necessary in the callee and IPA-SRA is removing it + from both functions. The statement argument will be removed during + call redirection, we just need a placeholder here, which we will + create by using the default def of the VAR_DECL already created as a + base to thos SSA names of the removed parameter which remain in the + function (original non-default-defs). */ + *expr_p = get_or_create_ssa_default_def (cfun, *r); + return true; + } + else + return modify_expression (expr_p, true); +} + /* 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 statement needs to be rebuilt, do so. Return true if any modifications have @@ -1676,7 +1798,7 @@ ipa_param_body_adjustments::modify_call_stmt (gcall **stmt_p) else { tree t = gimple_call_arg (stmt, i); - modify_expression (&t, true); + modify_call_argument (&t); vargs.safe_push (t); } } @@ -1700,7 +1822,7 @@ 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); + modified |= modify_call_argument (t); } if (gimple_call_lhs (stmt)) diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h index c80e1bc5d6b..2700028537f 100644 --- a/gcc/ipa-param-manipulation.h +++ b/gcc/ipa-param-manipulation.h @@ -370,6 +370,13 @@ public: /* Set to true if there are any IPA_PARAM_OP_SPLIT adjustments among stored adjustments. */ bool m_split_modifications_p; + + /* Statements and SSA_NAMEs that only manipulate data from parameters removed + because they are not necessary. SSA_NAMES are mapped to the replacement + VAR_DECL of the removed argument. */ + hash_set m_dead_stmts; + hash_map m_dead_ssas; + private: void common_initialization (tree old_fndecl, tree *vars, vec *tree_map); @@ -379,10 +386,12 @@ private: unsigned unit_offset); tree replace_removed_params_ssa_names (tree old_name, gimple *stmt); bool modify_expression (tree *expr_p, bool convert); + bool modify_call_argument (tree *expr_p); bool modify_assignment (gimple *stmt, gimple_seq *extra_stmts); bool modify_call_stmt (gcall **stmt_p); bool modify_cfun_body (); void reset_debug_stmts (); + void mark_dead_statements (tree dead_param, tree repl); /* 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 1dcb31c0267..8922ecbb52a 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -1528,6 +1528,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 @@ -1792,10 +1797,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); @@ -2674,7 +2684,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)