From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id C867B3857B98 for ; Fri, 1 Sep 2023 13:32:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C867B3857B98 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 031DC1F853; Fri, 1 Sep 2023 13:32:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1693575127; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=S4b7O9B37iBc87kKDXuxoh3JMTQwRzmQm6XsbL+66L8=; b=iUlL7eaPId5eXzRwjICUYiW6HCGxcl7ayvWzSDJ53klk+WclLUbF+LESUHz0K1t/tZEgs7 Su+pbgEDgF7vYk/3yP/ECUJxHUjtBTo0mT2hWaSdU/ga0lk4QviCaKTRTABJbNOJzTX+PI rzjVBcchyY2K7DKPsX1xyRsWkNTlnXc= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1693575127; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=S4b7O9B37iBc87kKDXuxoh3JMTQwRzmQm6XsbL+66L8=; b=DANt1V84oa0sbSp0k7ILpQ0IHaffjKzIF/xM0i2UsxlkUl24GiaRuEjHxKFaiglvQzTsO1 0E3evAYanO1mn/CA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id EB38213582; Fri, 1 Sep 2023 13:32:06 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id kodnOdbn8WQ9QwAAMHmgww (envelope-from ); Fri, 01 Sep 2023 13:32:06 +0000 From: Martin Jambor To: GCC Patches Cc: Jan Hubicka Subject: Re: [PATCH] ipa: Self-DCE of uses of removed call LHSs (PR 108007) In-Reply-To: References: User-Agent: Notmuch/0.37 (https://notmuchmail.org) Emacs/28.2 (x86_64-suse-linux-gnu) Date: Fri, 01 Sep 2023 15:32:06 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_SOFTFAIL,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 > > 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 > > 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 *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 > + *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 *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 *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 *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 (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 *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 (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 (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