From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113565 invoked by alias); 16 May 2019 14:04:49 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 113557 invoked by uid 89); 16 May 2019 14:04:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,KAM_SHORT,SPF_PASS autolearn=ham version=3.3.1 spammy=H*UA:https, engage X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 16 May 2019 14:04:46 +0000 X-Amavis-Alert: BAD HEADER SECTION, Duplicate header field: "Cc" Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 7B01EAC23; Thu, 16 May 2019 14:04:43 +0000 (UTC) From: Martin Jambor To: Richard Biener Cc: GCC Patches , Jan Hubicka Cc: Subject: Re: [PATCH] True IPA reimplementation of IPA-SRA In-Reply-To: References: User-Agent: Notmuch/0.28.4 (https://notmuchmail.org) Emacs/26.2 (x86_64-suse-linux-gnu) Date: Thu, 16 May 2019 14:04:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00955.txt.bz2 Hi Richi, On Thu, May 16 2019, Richard Biener wrote: > On Fri, May 10, 2019 at 10:31 AM Martin Jambor wrote: >> >> Hello, >> >> this is a follow-up from a WIP patch I sent here in late December: >> https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01765.html >> >> Just like the last time, the patch below is is a reimplementation of >> IPA-SRA to make it a full IPA pass that can handle strictly connected >> components in the call-graph, can take advantage of LTO and does not >> weirdly switch functions in pass-pipeline like our current quasi-IPA SRA >> does. Unlike the current IPA-SRA it can also remove return values, even >> in SCCs. On the other hand, it is less powerful when it comes to >> structures passed by reference. By design it will not create references >> to bits of an aggregate because that turned out to be just obfuscation >> in practice. However, it also cannot usually split aggregates passed by >> reference that are just passed to another function (where splitting >> would be useful) because it cannot perform the same TBAA analysis like >> the current implementation which already knows what types it should look >> at because it has access to bodies of all functions attempts to modify. > > So that's just because the analysis is imperfect? I mean if we can handle > > foo (X *p) { do_something (p->a); } > X a; a.a = 1; foo (&a); > > then we should be able to handle > > bar (X *p) { foo (p); } > X a; a.a = 1; bar (&a); So because the call to foo dominates EXIT and uses default definition MEM SSA, this example would be handled fine by the patch. But it cannot handle for example (assuming p->a is an int): bar (X *p) { *global_double_ptr = 0.0; foo (p); } The current IPA-SRA can, because at the time it looks at foo, bar has been already processed, and so it knows the load is of integer type. If necessary, we could try TBAA for fields in X if there is a reasonable number of them and at IPA level just check a flag saying that bar does not engage in some type-punning. Another example would be something like: bar (X *p) { if (cond) bar (p); else do_something_else (p->a); } The problem here is that the check if p is sure to be dereferenced when bar is called is also done at the intra-procedural level. Well, it is not actually a test if p is dereferenced but if the offset from p which is known to be dereferenced covers p->a. We could do it symbolically, arrive at some expression of the form MIN(offsetof(a), known_dereference_offset_in (bar)) store that to the IPA summary and then evaluate at IPA level. If we think that it is worth it. Still, I don't think the situation is that much worse in practice because IPA-SRA can only handle fairly simple cases anyway, and those are actually often taken care of by indirect inlining. > > Thanks for doing this. I wonder how difficult it is to split the > patch into a) old IPA-SRA removal, b) refactoring c) IPA-SRA add > (probably easiest in that order). It's quite a large number of > changes, a) being mostly uninteresting (and pre-approved hereby, just > not independently, of course), b) is uninteresting to me, but I would > like to look at c), not sure if that's really only the new file, > probably not since IPA modifications have infrastructure bits. The analysis parts of the new IPA-SRA, both the intra-procedural and inter-procedural are entirely in the new file ipa-sra.c so if you want to review that, just grab that file from https://gcc.gnu.org/git/?p=gcc.git;a=tree;h=refs/heads/jamborm/ipa-sra;hb=refs/heads/jamborm/ipa-sra The transformation part, however, are what the "refactoring" is really about because it is not the pass but rather the cgraph cloning infrastructure that performs the actual transformation. This is so on purpose because not only the bodies of changed functions need to be adjusted but also all calls to them, and you cannot register a pass-specific transformation function for that - and I need to actually pass information from the body transformation to the call transformation anyway. So yes, this split would be possible and perhaps even easy but it would not make much of a difference. Thanks, Martin > > Sorry for not mentioning earlier. Maybe just splitting out a) already > helps (you seem to remove code not in tree-sra.c). > > Thanks, > Richard. > >> Martin >> >> >> >> 2019-05-09 Martin Jambor >> >> * coretypes.h (cgraph_edge): Declare. >> * ipa-param-manipulation.c: Rewrite. >> * ipa-param-manipulation.h: Likewise. >> * Makefile.in (GTFILES): Added ipa-param-manipulation.h and ipa-sra.c. >> (OBJS): Added ipa-sra.o. >> * cgraph.h (ipa_replace_map): Removed fields old_tree, replace_p >> and ref_p, added fields param_adjustments and performed_splits. >> (struct cgraph_clone_info): Remove ags_to_skip and >> combined_args_to_skip, new field param_adjustments. >> (cgraph_node::create_clone): Changed parameters to use >> ipa_param_adjustments. >> (cgraph_node::create_virtual_clone): Likewise. >> (cgraph_node::create_virtual_clone_with_body): Likewise. >> (tree_function_versioning): Likewise. >> (cgraph_build_function_type_skip_args): Removed. >> * cgraph.c (cgraph_edge::redirect_call_stmt_to_callee): Convert to >> using ipa_param_adjustments. >> (clone_of_p): Likewise. >> * cgraphclones.c (cgraph_build_function_type_skip_args): Removed. >> (build_function_decl_skip_args): Likewise. >> (duplicate_thunk_for_node): Adjust parameters using >> ipa_param_body_adjustments, copy param_adjustments instead of >> args_to_skip. >> (cgraph_node::create_clone): Convert to using ipa_param_adjustments. >> (cgraph_node::create_virtual_clone): Likewise. >> (cgraph_node::create_version_clone_with_body): Likewise. >> (cgraph_materialize_clone): Likewise. >> (symbol_table::materialize_all_clones): Likewise. >> * ipa-fnsummary.c (ipa_fn_summary_t::duplicate): Simplify >> ipa_replace_map check. >> * ipa-cp.c (get_replacement_map): Do not initialize removed fields. >> (initialize_node_lattices): Make aware that some parameters might have >> already been removed. >> (want_remove_some_param_p): New function. >> (create_specialized_node): Convert to using ipa_param_adjustments and >> deal with possibly pre-existing adjustments. >> * lto-cgraph.c (output_cgraph_opt_summary_p): Likewise. >> (output_node_opt_summary): Do not stream removed fields. Stream >> parameter adjustments instead of argumetns to skip. >> (input_node_opt_summary): Likewise. >> (input_node_opt_summary): Likewise. >> * lto-section-in.c (lto_section_name): Added ipa-sra section. >> * lto-streamer.h (lto_section_type): Likewise. >> * tree-inline.h (copy_body_data): New field killed_new_ssa_names. >> (copy_decl_to_var): Declare. >> * tree-inline.c (update_clone_info): Do not remap old_tree. >> (remap_gimple_stmt): Use ipa_param_body_adjustments to modify gimple >> statements, walk all extra generated statements and remap their >> operands. >> (redirect_all_calls): Add killed SSA names to a hash set. >> (remap_ssa_name): Do not remap killed SSA names. >> (copy_arguments_for_versioning): Renames to copy_arguments_nochange, >> half of functionality moved to ipa_param_body_adjustments. >> (copy_decl_to_var): Make exported. >> (copy_body): Destroy killed_new_ssa_names hash set. >> (expand_call_inline): Remap performed splits. >> (update_clone_info): Likewise. >> (tree_function_versioning): Simplify tree_map processing. Updated to >> accept ipa_param_adjustments and use ipa_param_body_adjustments. >> * tree-inline.h (struct copy_body_data): New field param_body_adjs. >> * omp-simd-clone.c (simd_clone_vector_of_formal_parm_types): Adjust >> for the new interface. >> (simd_clone_clauses_extract): Likewise, make args an auto_vec. >> (simd_clone_compute_base_data_type): Likewise. >> (simd_clone_init_simd_arrays): Adjust for the new interface. >> (simd_clone_adjust_argument_types): Likewise. >> (struct modify_stmt_info): Likewise. >> (ipa_simd_modify_stmt_ops): Likewise. >> (ipa_simd_modify_function_body): Likewise. >> (simd_clone_adjust): Likewise. >> * tree-sra.c: Removed IPA-SRA. Include tree-sra.h. >> (type_internals_preclude_sra_p): Make public. >> * tree-sra.h: New file. >> * ipa-inline-transform.c (save_inline_function_body): Update to >> refelct new tree_function_versioning signature. >> * ipa-prop.c (adjust_agg_replacement_values): Use a helper from >> (ipcp_modif_dom_walker::before_dom_children): Likewise. >> ipa_param_adjustments to get current parameter indices. >> (ipcp_update_bits): Likewise. >> (ipcp_update_vr): Likewise. >> * ipa-split.c (split_function): Convert to using ipa_param_adjustments. >> * ipa-sra.c: New file. >> * multiple_target.c (create_target_clone): Update to reflet new type >> of create_version_clone_with_body. >> * trans-mem.c (ipa_tm_create_version): Update to reflect new type of >> tree_function_versioning. >> * tree-sra.c (modify_function): Update to reflect new type of >> tree_function_versioning. >> * params.def (PARAM_IPA_SRA_MAX_REPLACEMENT): New. >> (PARAM_SRA_MAX_TYPE_CHECK_STEPS): New. >> * passes.def: Remove old IPA-SRA and add new one. >> * tree-pass.h (make_pass_early_ipa_sra): Remove declaration. >> (make_pass_ipa_sra): Declare. >> >> testsuite/ >> * g++.dg/ipa/pr81248.C: Adjust dg-options and dump-scan. >> * gcc.dg/ipa/ipa-sra-1.c: Likewise. >> * gcc.dg/ipa/ipa-sra-10.c: Likewise. >> * gcc.dg/ipa/ipa-sra-11.c: Likewise. >> * gcc.dg/ipa/ipa-sra-3.c: Likewise. >> * gcc.dg/ipa/ipa-sra-4.c: Likewise. >> * gcc.dg/ipa/ipa-sra-5.c: Likewise. >> * gcc.dg/ipa/ipacost-2.c: Disable ipa-sra. >> * gcc.dg/ipa/ipcp-agg-9.c: Likewise. >> * gcc.dg/ipa/pr78121.c: Adjust scan pattern. >> * gcc.dg/ipa/vrp1.c: Likewise. >> * gcc.dg/ipa/vrp2.c: Likewise. >> * gcc.dg/ipa/vrp3.c: Likewise. >> * gcc.dg/ipa/vrp7.c: Likewise. >> * gcc.dg/ipa/vrp8.c: Likewise. >> * gcc.dg/noreorder.c: use noipa attribute instead of noinline. >> * gcc.dg/ipa/20040703-wpa.c: New test. >> * gcc.dg/ipa/ipa-sra-12.c: New test. >> * gcc.dg/ipa/ipa-sra-13.c: Likewise. >> * gcc.dg/ipa/ipa-sra-14.c: Likewise. >> * gcc.dg/ipa/ipa-sra-15.c: Likewise. >> * gcc.dg/ipa/ipa-sra-16.c: Likewise. >> * gcc.dg/ipa/ipa-sra-17.c: Likewise. >> * gcc.dg/ipa/ipa-sra-18.c: Likewise. >> * gcc.dg/ipa/ipa-sra-19.c: Likewise. >> * gcc.dg/ipa/ipa-sra-20.c: Likewise. >> * gcc.dg/ipa/ipa-sra-21.c: Likewise. >> * gcc.dg/sso/ipa-sra-1.c: Likewise. >> >> * gcc.dg/ipa/ipa-sra-2.c: Mark to be removed. >> * gcc.dg/ipa/ipa-sra-6.c: Likewise. >> >>