From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 7AF8E3864823 for ; Thu, 15 Jul 2021 16:04:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7AF8E3864823 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id AFD402829C7; Thu, 15 Jul 2021 18:04:33 +0200 (CEST) Date: Thu, 15 Jul 2021 18:04:33 +0200 From: Jan Hubicka To: Martin Jambor Cc: GCC Patches , Martin Liska Subject: Re: [RFC] ipa: Adjust references to identify read-only globals Message-ID: <20210715160433.GB15377@kam.mff.cuni.cz> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: , X-List-Received-Date: Thu, 15 Jul 2021 16:04:37 -0000 > Hi, > > gcc/ChangeLog: > > 2021-06-29 Martin Jambor > > * cgraph.h (ipa_replace_map): New field force_load_ref. > * ipa-prop.h (ipa_param_descriptor): Reduce precision of move_cost, > aded new flag load_dereferenced, adjusted comments. > (ipa_get_param_dereferenced): New function. > (ipa_set_param_dereferenced): Likewise. > * cgraphclones.c (cgraph_node::create_virtual_clone): Follow it. > * ipa-cp.c: Include gimple.h. > (ipcp_discover_new_direct_edges): Take into account dereferenced flag. > (get_replacement_map): New parameter force_load_ref, set the > appropriate flag in ipa_replace_map if set. > (struct symbol_and_index_together): New type. > (adjust_references_in_act_callers): New function. > (adjust_references_in_caller): Likewise. > (create_specialized_node): When appropriate, call > adjust_references_in_caller and force only load references. > * ipa-prop.c (load_from_dereferenced_name): New function. > (ipa_analyze_controlled_uses): Also detect loads from a > dereference, harden testing of call statements. > (ipa_write_node_info): Stream the dereferenced flag. > (ipa_read_node_info): Likewise. > (ipa_set_jf_constant): Also create refdesc when jump function > references a variable. > (cgraph_node_for_jfunc): Rename to symtab_node_for_jfunc, work > also on references of variables and return a symtab_node. Adjust > all callers. > (propagate_controlled_uses): Also remove references to VAR_DECLs. > > gcc/testsuite/ChangeLog: > > 2021-06-29 Martin Jambor > > * gcc.dg/ipa/remref-3.c: New test. > * gcc.dg/ipa/remref-4.c: Likewise. > * gcc.dg/ipa/remref-5.c: Likewise. > * gcc.dg/ipa/remref-6.c: Likewise. > --- > gcc/cgraph.h | 3 + > gcc/cgraphclones.c | 10 +- > gcc/ipa-cp.c | 146 ++++++++++++++++++++++-- > gcc/ipa-prop.c | 166 ++++++++++++++++++++++------ > gcc/ipa-prop.h | 27 ++++- > gcc/testsuite/gcc.dg/ipa/remref-3.c | 23 ++++ > gcc/testsuite/gcc.dg/ipa/remref-4.c | 31 ++++++ > gcc/testsuite/gcc.dg/ipa/remref-5.c | 38 +++++++ > gcc/testsuite/gcc.dg/ipa/remref-6.c | 24 ++++ > 9 files changed, 419 insertions(+), 49 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/remref-3.c > create mode 100644 gcc/testsuite/gcc.dg/ipa/remref-4.c > create mode 100644 gcc/testsuite/gcc.dg/ipa/remref-5.c > create mode 100644 gcc/testsuite/gcc.dg/ipa/remref-6.c > > diff --git a/gcc/cgraph.h b/gcc/cgraph.h > index 9f4338fdf87..0fc20cd4517 100644 > --- a/gcc/cgraph.h > +++ b/gcc/cgraph.h > @@ -700,6 +700,9 @@ struct GTY(()) ipa_replace_map > tree new_tree; > /* Parameter number to replace, when old_tree is NULL. */ > int parm_num; > + /* Set if the newly added reference should not be an address one, but a load > + one from the operand of the ADDR_EXPR in NEW_TREE. */ So this is in case where parameter p is used only as *p? I think the comment should be expanded to explain the situation or in a year I will not know why we need such a flag :) > @@ -4320,7 +4322,8 @@ gather_edges_for_value (ipcp_value *val, cgraph_node *dest, > Return it or NULL if for some reason it cannot be created. */ > > static struct ipa_replace_map * > -get_replacement_map (class ipa_node_params *info, tree value, int parm_num) > +get_replacement_map (class ipa_node_params *info, tree value, int parm_num, > + bool force_load_ref) You want to comment the parameter here too.. > +/* At INDEX of a function being called by CS there is an ADDR_EXPR of a > + variable which is only dereferenced and which is represented by SYMBOL. See > + if we can remove ADDR reference in callers assosiated witht the call. */ > + > +static void > +adjust_references_in_caller (cgraph_edge *cs, symtab_node *symbol, int index) > +{ > + ipa_edge_args *args = ipa_edge_args_sum->get (cs); > + ipa_jump_func *jfunc = ipa_get_ith_jump_func (args, index); > + if (jfunc->type == IPA_JF_CONST) > + { > + ipa_ref *to_del = cs->caller->find_reference (symbol, cs->call_stmt, > + cs->lto_stmt_uid); > + if (!to_del) > + return; > + to_del->remove_reference (); > + if (dump_file) > + fprintf (dump_file, " Removed a reference from %s to %s.\n", > + cs->caller->dump_name (), symbol->dump_name ()); > + return; > + } > + > + if (jfunc->type != IPA_JF_PASS_THROUGH > + || ipa_get_jf_pass_through_operation (jfunc) != NOP_EXPR) > + return; > + > + int fidx = ipa_get_jf_pass_through_formal_id (jfunc); > + cgraph_node *caller = cs->caller; > + ipa_node_params *caller_info = ipa_node_params_sum->get (caller); > + /* TODO: This consistency check may be too big and not really > + that useful. Consider removing it. */ > + tree cst; > + if (caller_info->ipcp_orig_node) > + cst = caller_info->known_csts[fidx]; > + else > + { > + ipcp_lattice *lat = ipa_get_scalar_lat (caller_info, fidx); > + gcc_assert (lat->is_single_const ()); > + cst = lat->values->value; > + } > + gcc_assert (TREE_CODE (cst) == ADDR_EXPR > + && symtab_node::get (TREE_OPERAND (cst, 0)) == symbol); > + > + int cuses = ipa_get_controlled_uses (caller_info, fidx); > + if (cuses == IPA_UNDESCRIBED_USE) > + return; > + gcc_assert (cuses > 0); > + cuses--; > + ipa_set_controlled_uses (caller_info, fidx, cuses); > + if (cuses) > + return; > + > + if (caller_info->ipcp_orig_node) > + { > + /* Cloning machinery has created a reference here, we need to either > + remove it or change it to a read one. */ > + ipa_ref *to_del = caller->find_reference (symbol, NULL, 0); > + if (to_del && to_del->use == IPA_REF_ADDR) > + { > + to_del->remove_reference (); > + if (dump_file) > + fprintf (dump_file, " Removed a reference from %s to %s.\n", > + cs->caller->dump_name (), symbol->dump_name ()); > + if (ipa_get_param_load_dereferenced (caller_info, fidx)) > + { > + caller->create_reference (symbol, IPA_REF_LOAD, NULL); > + if (dump_file) > + fprintf (dump_file, > + " ...and replaced it with LOAD one.\n"); > + } > + } > + } > + > + symbol_and_index_together pack; > + pack.symbol = symbol; > + pack.index = fidx; > + caller->call_for_symbol_thunks_and_aliases (adjust_references_in_act_callers, > + &pack, true); > +} > + > + > /* Return true if we would like to remove a parameter from NODE when cloning it > with KNOWN_CSTS scalar constants. */ > > @@ -4595,15 +4708,28 @@ create_specialized_node (struct cgraph_node *node, > for (i = 0; i < count; i++) > { > tree t = known_csts[i]; > - if (t) > - { > - struct ipa_replace_map *replace_map; > + if (!t) > + continue; > > - gcc_checking_assert (TREE_CODE (t) != TREE_BINFO); > - replace_map = get_replacement_map (info, t, i); > - if (replace_map) > - vec_safe_push (replace_trees, replace_map); > + gcc_checking_assert (TREE_CODE (t) != TREE_BINFO); > + > + bool load_ref = false; > + symtab_node *ref_symbol; > + if (TREE_CODE (t) == ADDR_EXPR > + && TREE_CODE (TREE_OPERAND (t, 0)) == VAR_DECL Can't you see more complicate expressions like &var[10]? So you want to find the base term here I think. > +/* Return true EXPR is a load from a dereference of SSA_NAME NAME. */ > + > +static bool > +load_from_dereferenced_name (tree expr, tree name) > +{ > + tree base = get_base_address (expr); > + return (TREE_CODE (base) == MEM_REF > + && TREE_OPERAND (base, 0) == name); > +} Similarly here. > /* The parameter is used. */ > unsigned used : 1; > unsigned used_by_ipa_predicates : 1; > unsigned used_by_indirect_call : 1; > unsigned used_by_polymorphic_call : 1; > + /* Set to true when in addition to being used in call statements, the > + parameterr has also been used for loads (but not for wtires, does not > + escape, etc.). */ > + unsigned load_dereferenced : 1; Some typos here :) (and also I think comment should explain why we need the flag) The patch looks reasonable to me, Honza