From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id CE57C3858410 for ; Tue, 19 Sep 2023 11:37:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CE57C3858410 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass 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 E14791FE31; Tue, 19 Sep 2023 11:36:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1695123419; 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=9Tv1BZx381CtnujQ9TxeG9j2+fWcDNsEtmq5Df9am5Y=; b=K6aWaGgQzhjHWIIHLU0af8NlFwYY9eQTvW63PvnOw0Azq9NWqsY6DGoc9gavOqG0PI4Jxh ivmxhbZBYC4A0Ize/+svB/bWD8KG+2mkPVKoL5Hl83eb3gkz8cotH7DwMwktxSEjvgnuYg d6GgldTDx+OTMGcRu0KrZm/ZKH3Lz7c= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1695123419; 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=9Tv1BZx381CtnujQ9TxeG9j2+fWcDNsEtmq5Df9am5Y=; b=4g8DUZqLNb/W551ih/Tws75GGLwwwmtIWVlSZa5SdIVDbCKd15a7Np5wXQfCcbwrEIn6Gl UJE3Ic7EcDp+lWBQ== 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 D4B41134F3; Tue, 19 Sep 2023 11:36:59 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id Qk7jM9uHCWWZcwAAMHmgww (envelope-from ); Tue, 19 Sep 2023 11:36:59 +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/29.1 (x86_64-suse-linux-gnu) Date: Tue, 19 Sep 2023 13:36:59 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,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, 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 >> >> 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