* [PATCH] True IPA reimplementation of IPA-SRA @ 2019-05-10 8:31 Martin Jambor 2019-05-16 12:09 ` Richard Biener 2019-06-18 12:00 ` Martin Liška 0 siblings, 2 replies; 7+ messages in thread From: Martin Jambor @ 2019-05-10 8:31 UTC (permalink / raw) To: GCC Patches; +Cc: Jan Hubicka [-- Attachment #1: Type: text/plain, Size: 8367 bytes --] 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. Since the last time I have fixed a number of bugs that Martin Liška found when compiling a portion of openSUSE with the patch, removed all the FIXMEs, made long living memory structures more compact and self-reviewed the entire patch once. Therefore, I would like to ask for a review and eventually for an approval to commit the patch to the trunk. The patch survives bootstrap, LTO bootstrap and LTO profiledbootstrap on x86_64-linux. In the testsuite, it "fixes" 24 guality passes (all LTO ones) but breaks 12 other ones (one is non-LTO). I would welcome any help with addressing these. Because the patch removes the old IPA-SRA, the input to the IPA pipeline looks different and so I could not just try to make it "process the debug statements like before." Because the patch is big I had to compress it to get it through to gcc-patches. Because of its size and because it contains a completely new file ipa-sra.c and total reimplementations of ipa-param-manipulation.[ch], and so I pushed my development branch to branch jamborm/ipa-sra on GCC git server (https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/jamborm/ipa-sra). It may be more convenient to check it out and review it that way. Thanks in advance for any questions, comments and suggestions, Martin 2019-05-09 Martin Jambor <mjambor@suse.cz> * 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. [-- Attachment #2: ipa-sra-190509.diff.bz2 --] [-- Type: application/x-bzip2, Size: 74784 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] True IPA reimplementation of IPA-SRA 2019-05-10 8:31 [PATCH] True IPA reimplementation of IPA-SRA Martin Jambor @ 2019-05-16 12:09 ` Richard Biener 2019-05-16 14:04 ` Martin Jambor 2019-06-18 12:00 ` Martin Liška 1 sibling, 1 reply; 7+ messages in thread From: Richard Biener @ 2019-05-16 12:09 UTC (permalink / raw) To: Martin Jambor; +Cc: GCC Patches, Jan Hubicka On Fri, May 10, 2019 at 10:31 AM Martin Jambor <mjambor@suse.cz> 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); by just applying local analysis of foo when looking at what to do for bar? So isn't that what a IPA propagation step should do? > Since the last time I have fixed a number of bugs that Martin Liška > found when compiling a portion of openSUSE with the patch, removed all > the FIXMEs, made long living memory structures more compact and > self-reviewed the entire patch once. > > Therefore, I would like to ask for a review and eventually for an > approval to commit the patch to the trunk. The patch survives > bootstrap, LTO bootstrap and LTO profiledbootstrap on x86_64-linux. In > the testsuite, it "fixes" 24 guality passes (all LTO ones) but breaks 12 > other ones (one is non-LTO). I would welcome any help with addressing > these. Because the patch removes the old IPA-SRA, the input to the IPA > pipeline looks different and so I could not just try to make it "process > the debug statements like before." > > Because the patch is big I had to compress it to get it through to > gcc-patches. Because of its size and because it contains a completely > new file ipa-sra.c and total reimplementations of > ipa-param-manipulation.[ch], and so I pushed my development branch to > branch jamborm/ipa-sra on GCC git server > (https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/jamborm/ipa-sra). > It may be more convenient to check it out and review it that way. > > Thanks in advance for any questions, comments and suggestions, 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. 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 <mjambor@suse.cz> > > * 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. > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] True IPA reimplementation of IPA-SRA 2019-05-16 12:09 ` Richard Biener @ 2019-05-16 14:04 ` Martin Jambor 2019-05-16 18:38 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Martin Jambor @ 2019-05-16 14:04 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches, Jan Hubicka Hi Richi, On Thu, May 16 2019, Richard Biener wrote: > On Fri, May 10, 2019 at 10:31 AM Martin Jambor <mjambor@suse.cz> 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 <mjambor@suse.cz> >> >> * 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. >> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] True IPA reimplementation of IPA-SRA 2019-05-16 14:04 ` Martin Jambor @ 2019-05-16 18:38 ` Richard Biener 2019-06-13 14:26 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2019-05-16 18:38 UTC (permalink / raw) To: Martin Jambor; +Cc: GCC Patches, Jan Hubicka On Thu, May 16, 2019 at 4:04 PM Martin Jambor <mjambor@suse.cz> wrote: > > Hi Richi, > > On Thu, May 16 2019, Richard Biener wrote: > > On Fri, May 10, 2019 at 10:31 AM Martin Jambor <mjambor@suse.cz> 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. Ah, I see. It is of course sth we could analyze locally and propagate, like forming an access tree for each parameters much like SRA collects it (eventually marking sub-accesses that are always performed). > 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. Agreed. I suppose the new pass is OK as-is feature wise and we can always enhance it later if we figure out it is worth it. > > 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. OK, thanks for explaining - I will look at the new file then. Richard. > 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 <mjambor@suse.cz> > >> > >> * 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. > >> > >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] True IPA reimplementation of IPA-SRA 2019-05-16 18:38 ` Richard Biener @ 2019-06-13 14:26 ` Jan Hubicka 2019-06-26 14:46 ` Martin Jambor 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2019-06-13 14:26 UTC (permalink / raw) To: Richard Biener; +Cc: Martin Jambor, GCC Patches, Jan Hubicka Hi, i read all changes except for ipa-sra itself. Here are some comments, I will look at the remaining file next. Honza diff --git a/gcc/cgraph.h b/gcc/cgraph.h index 9a19d83fffb..3f838c08e76 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h struct GTY(()) cgraph_clone_info { + /* Constants discovered by IPA-CP, i.e. which parameter should be replaced + with what. */ vec<ipa_replace_map *, va_gc> *tree_map; - bitmap args_to_skip; - bitmap combined_args_to_skip; + /* Parameter modification that IPA-SRA decided to perform. */ + ipa_param_adjustments *param_adjustments; + /* Lists of all splits with their offsets for each dummy variables + representing a replaced-by-splits parameter. */ + vec<ipa_param_performed_split, va_gc> *performed_splits; Please explain what dummy variables are :) +/* Return true if we would like to remove a parameter from NODE when cloning it + with KNOWN_CSTS scalar constants. */ + +static bool +want_remove_some_param_p (cgraph_node *node, vec<tree> known_csts) +{ + auto_vec<bool, 16> surviving; + bool filled_vec = false; + ipa_node_params *info = IPA_NODE_REF (node); + int i, count = ipa_get_param_count (info); + for (i = 0; i < count; i++) vertical space after the declarations :) - tree t = known_csts[i]; - - if (t || !ipa_is_param_used (info, i)) - bitmap_set_bit (args_to_skip, i); + ipa_adjusted_param *old_adj = &(*old_adjustments->m_adj_params)[i]; + if (!node->local.can_change_signature + || old_adj->op != IPA_PARAM_OP_COPY + || (!known_csts[old_adj->base_index] + && ipa_is_param_used (info, old_adj->base_index))) + { + ipa_adjusted_param new_adj; + memcpy (&new_adj, old_adj, sizeof (new_adj)); Why this is not *new_adj=*old_adj? +/* Names of parameters for dumping. Keep in sync with enum ipa_parm_op. */ + +static const char *ipa_param_op_names[] = {"IPA_PARAM_OP_UNDEFINED", + "IPA_PARAM_OP_COPY", + "IPA_PARAM_OP_NEW", + "IPA_PARAM_OP_SPLIT"}; Given brave new C++ world, can't we statically assert that size of array match the enum? Also it seems to me that ipa-param-modification would benefit from some toplevel comment of what it does and what is the main API how to use it. Also functions like ipa_fill_vector_with_formal_parms ipa_fill_vector_with_formal_parm_types does not seem very IPA specific and it was not quite obvoius from name what it does. Perhaps it should go somewhere to common tree manipulatoin and have more fitting name? If it was something like push_function_arg_decls or push_function_arg_types it would be more obvious to me what it does :) Also I wonder if the code would not be more readable if functions was returning auto_vecs references. Then they can be just something like "function_arg_types" and the API would be self explanatory. +tree +ipa_param_adjustments::adjust_decl (tree orig_decl) +{ + tree new_decl = copy_node (orig_decl); + tree orig_type = TREE_TYPE (orig_decl); + if (prototype_p (orig_type) + || (m_skip_return && !VOID_TYPE_P (TREE_TYPE (orig_type)))) + { + tree new_type = build_new_function_type (orig_type, false); + TREE_TYPE (new_decl) = new_type; + } + if (method2func_p (orig_type)) + DECL_VINDEX (new_decl) = NULL_TREE; I think you want to clear DECL_VINDEX in every case since the method is no longer one pointed to by virtual table. But I see we have similar code elsewhere so lets do that incrementally. With early debug info we probably can forget about it completely. +/* Structure to hold declarations representing transitive IPA-SRA splits. In + essence, if we need to pass UNIT_OFFSET of a parameter which originally has + number BASE_INDEX, we should pass down REPL. */ + +struct transitive_split_map +{ + tree repl; + unsigned base_index; + unsigned unit_offset; +}; It is not quite clear to me what those transitive splits are, so perhaps better description would help :) + gimple_stmt_iterator gsi = gsi_for_stmt (stmt); + gimple_stmt_iterator prev_gsi = gsi; + gsi_prev (&prev_gsi); I think we still maintain vertical space after declarations at the beggining of block. It may help to break out this function a bit. With all the debug handling it is now quite monster. +/* 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 + a chained list of new local variables. TREE_MAP is the IPA-CP produced + mapping of trees to constants. */ + +void +ipa_param_body_adjustments::common_initialization (tree old_fndecl, + tree *vars, + vec<ipa_replace_map *, + va_gc> *tree_map) Some comments int he body about what the code does would help IMO :) It is bit of spagetti. diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h index 71fc4a201aa..85aa90e0403 100644 --- a/gcc/ipa-param-manipulation.h +++ b/gcc/ipa-param-manipulation.h @@ -21,96 +21,282 @@ along with GCC; see the file COPYING3. If not see #ifndef IPA_PARAM_MANIPULATION_H #define IPA_PARAM_MANIPULATION_H +/* Indices into ipa_param_prefixes to identify a human-readable prefix for newly + synthesized parameters. Keep in sync with the array. */ +#define IPA_PARAM_PREFIX_SYNTH 0 +#define IPA_PARAM_PREFIX_ISRA 1 +#define IPA_PARAM_PREFIX_SIMD 2 +#define IPA_PARAM_PREFIX_MASK 3 Probably better as enum? diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c index 5eaf8257f98..223dfeee7f9 100644 --- a/gcc/ipa-split.c +++ b/gcc/ipa-split.c Eventually we want ot teach ipa-split to do more adjustments, like adding new parameters for values computed in header that needs to be passed to tail :) diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 9bf1c4080f5..88895105bdc 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -191,7 +190,17 @@ remap_ssa_name (tree name, copy_body_data *id) n = id->decl_map->get (name); if (n) - return unshare_expr (*n); + { + if (id->killed_new_ssa_names + && id->killed_new_ssa_names->contains (*n)) + { + gcc_assert (processing_debug_stmt); + processing_debug_stmt = -1; + return name; + } + + return unshare_expr (*n); + } if (processing_debug_stmt) { @@ -1872,6 +1881,21 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id) gcc_assert (n); gimple_set_block (copy, *n); } + if (id->param_body_adjs) + { + gimple_seq extra_stmts = NULL; + id->param_body_adjs->modify_gimple_stmt (©, &extra_stmts); + if (!gimple_seq_empty_p (extra_stmts)) + { + memset (&wi, 0, sizeof (wi)); + wi.info = id; + for (gimple_stmt_iterator egsi = gsi_start (extra_stmts); + !gsi_end_p (egsi); + gsi_next (&egsi)) + walk_gimple_op (gsi_stmt (egsi), remap_gimple_op_r, &wi); + gimple_seq_add_seq (&stmts, extra_stmts); + } + } @@ -2794,10 +2818,20 @@ redirect_all_calls (copy_body_data * id, basic_block bb) gimple *stmt = gsi_stmt (si); if (is_gimple_call (stmt)) { + tree old_lhs = gimple_call_lhs (stmt); struct cgraph_edge *edge = id->dst_node->get_edge (stmt); if (edge) { edge->redirect_call_stmt_to_callee (); + if (old_lhs + && TREE_CODE (old_lhs) == SSA_NAME + && !gimple_call_lhs (edge->call_stmt)) + { + if (!id->killed_new_ssa_names) + id->killed_new_ssa_names = new hash_set<tree> (16); + id->killed_new_ssa_names->add (old_lhs); + } + Some enlightening comments would help for those three hunks :) So if LHS of call died you need to get rid of it. I wonder if one can't also just store 0 to it and let later cleanups to do their job? @@ -5653,7 +5721,7 @@ copy_decl_for_dup_finish (copy_body_data *id, tree decl, tree copy) return copy; } -static tree +tree copy_decl_to_var (tree decl, copy_body_data *id) { tree copy, type; Add a comment what function does, especially when it is exported now ;) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] True IPA reimplementation of IPA-SRA 2019-06-13 14:26 ` Jan Hubicka @ 2019-06-26 14:46 ` Martin Jambor 0 siblings, 0 replies; 7+ messages in thread From: Martin Jambor @ 2019-06-26 14:46 UTC (permalink / raw) To: Jan Hubicka; +Cc: GCC Patches, Richard Biener Hi, On Thu, Jun 13 2019, Jan Hubicka wrote: > Hi, > i read all changes except for ipa-sra itself. Here are some comments, > I will look at the remaining file next. > > Honza > > > diff --git a/gcc/cgraph.h b/gcc/cgraph.h > index 9a19d83fffb..3f838c08e76 100644 > --- a/gcc/cgraph.h > +++ b/gcc/cgraph.h > struct GTY(()) cgraph_clone_info > { > + /* Constants discovered by IPA-CP, i.e. which parameter should be > replaced > + with what. */ > vec<ipa_replace_map *, va_gc> *tree_map; > - bitmap args_to_skip; > - bitmap combined_args_to_skip; > + /* Parameter modification that IPA-SRA decided to perform. */ > + ipa_param_adjustments *param_adjustments; > + /* Lists of all splits with their offsets for each dummy variables > + representing a replaced-by-splits parameter. */ > + vec<ipa_param_performed_split, va_gc> *performed_splits; > > Please explain what dummy variables are :) OK, I replaced the comment with: /* Lists of dummy-decl and offset pairs representing split formal parameters in the caller. Offsets of all new replacements are enumerated, those coming from the same original parameter have the same dummy decl stored along with them. Dummy decls sit in call statement arguments followed by new parameter decls (or their SSA names) in between (caller) clone materialization and call redirection. Redirection then recognizes the dummy variable and together with the stored offsets can reconstruct what exactly the new parameter decls represent and can leave in place only those that the callee expects. */ > > +/* Return true if we would like to remove a parameter from NODE when > cloning it > + with KNOWN_CSTS scalar constants. */ > + > +static bool > +want_remove_some_param_p (cgraph_node *node, vec<tree> known_csts) > +{ > + auto_vec<bool, 16> surviving; > + bool filled_vec = false; > + ipa_node_params *info = IPA_NODE_REF (node); > + int i, count = ipa_get_param_count (info); > + for (i = 0; i < count; i++) > > vertical space after the declarations :) OK > > - tree t = known_csts[i]; > - > - if (t || !ipa_is_param_used (info, i)) > - bitmap_set_bit (args_to_skip, i); > + ipa_adjusted_param *old_adj = &(*old_adjustments->m_adj_params)[i]; > + if (!node->local.can_change_signature > + || old_adj->op != IPA_PARAM_OP_COPY > + || (!known_csts[old_adj->base_index] > + && ipa_is_param_used (info, old_adj->base_index))) > + { > + ipa_adjusted_param new_adj; > + memcpy (&new_adj, old_adj, sizeof (new_adj)); > > Why this is not *new_adj=*old_adj? Not sure, I changed it to aggregate copy. > > +/* Names of parameters for dumping. Keep in sync with enum > ipa_parm_op. */ > + > +static const char *ipa_param_op_names[] = {"IPA_PARAM_OP_UNDEFINED", > + "IPA_PARAM_OP_COPY", > + "IPA_PARAM_OP_NEW", > + "IPA_PARAM_OP_SPLIT"}; > > Given brave new C++ world, can't we statically assert that size of > array match the enum? Hm, I suppose so, I changed it. > Also it seems to me that ipa-param-modification would benefit from > some toplevel comment of what it does and what is the main API how > to use it. > Right, so I wrote that, but it ended up a bit large, have a look at: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/ipa-param-manipulation.h;h=b9da00bb5c9c813a0b16c7ec2f32e0d480f91238;hb=refs/heads/jamborm/ipa-sra > Also functions like > > ipa_fill_vector_with_formal_parms > ipa_fill_vector_with_formal_parm_types > > does not seem very IPA specific They are basically replacements of currently existing functions: vec<tree> ipa_get_vector_of_formal_parms (tree fndecl); vec<tree> ipa_get_vector_of_formal_parm_types (tree fntype); The difference is that the reimplementations do not directly return a new heap allocated vector but fill a vector that the caller passes, which makes it auto_vec friendly. > and it was not quite obvoius from name what it does. Perhaps it > should go somewhere to common tree manipulatoin and have more > fitting name? I'm not sure what the problem with the name is, but... > If it was something like push_function_arg_decls or > push_function_arg_types it would be more obvious to me what it does > :) ...if that is the case, OK, why not, I changed them. As far as their placement is concerned, again, I don't care much. I can put them to tree.[hc] if you'd like (I haven't dome that that yet). It just so happens that people only use these when they mess with parameters, hence they are in ipa-param-manipulation.[hc]. > > Also I wonder if the code would not be more readable if functions was > returning auto_vecs references. Then they can be just something like > "function_arg_types" and the API would be self explanatory. I probably don't understand. References to auto_vecs that are in fact stored where? > +tree > +ipa_param_adjustments::adjust_decl (tree orig_decl) > +{ > + tree new_decl = copy_node (orig_decl); > + tree orig_type = TREE_TYPE (orig_decl); > + if (prototype_p (orig_type) > + || (m_skip_return && !VOID_TYPE_P (TREE_TYPE (orig_type)))) > + { > + tree new_type = build_new_function_type (orig_type, false); > + TREE_TYPE (new_decl) = new_type; > + } > + if (method2func_p (orig_type)) > + DECL_VINDEX (new_decl) = NULL_TREE; > > I think you want to clear DECL_VINDEX in every case since the method > is no longer one pointed to by virtual table. But I see we have > similar code elsewhere so lets do that incrementally. > OK. I have noted it down. > With early debug info we probably can forget about it completely. I can try that later too. > +/* Structure to hold declarations representing transitive IPA-SRA > splits. In > + essence, if we need to pass UNIT_OFFSET of a parameter which > originally has > + number BASE_INDEX, we should pass down REPL. */ > + > +struct transitive_split_map > +{ > + tree repl; > + unsigned base_index; > + unsigned unit_offset; > +}; > > It is not quite clear to me what those transitive splits are, so perhaps > better description would help :) I have dedicated a large section of the initial comment linked above to the explanation of the term and how the patch handles such situations. > + gimple_stmt_iterator gsi = gsi_for_stmt (stmt); > + gimple_stmt_iterator prev_gsi = gsi; > + gsi_prev (&prev_gsi); > I think we still maintain vertical space after declarations at the > beggining > of block. It may help to break out this function a bit. With all the > debug > handling it is now quite monster. > +/* 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 > + a chained list of new local variables. TREE_MAP is the IPA-CP > produced > + mapping of trees to constants. */ > + > +void > +ipa_param_body_adjustments::common_initialization (tree old_fndecl, > + tree *vars, > + vec<ipa_replace_map *, > + va_gc> *tree_map) > > Some comments int he body about what the code does would help IMO :) > It is bit of spagetti. The function is rather long but it really only initializes all data members of the class. I tried to give the members clear names and descriptions so that it would be clear what is what. I tried to add some comments nevertheless. > > diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h > index 71fc4a201aa..85aa90e0403 100644 > --- a/gcc/ipa-param-manipulation.h > +++ b/gcc/ipa-param-manipulation.h > @@ -21,96 +21,282 @@ along with GCC; see the file COPYING3. If not see > #ifndef IPA_PARAM_MANIPULATION_H > #define IPA_PARAM_MANIPULATION_H > > +/* Indices into ipa_param_prefixes to identify a human-readable prefix > for newly > + synthesized parameters. Keep in sync with the array. */ > +#define IPA_PARAM_PREFIX_SYNTH 0 > +#define IPA_PARAM_PREFIX_ISRA 1 > +#define IPA_PARAM_PREFIX_SIMD 2 > +#define IPA_PARAM_PREFIX_MASK 3 > > Probably better as enum? OK. > diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c > index 5eaf8257f98..223dfeee7f9 100644 > --- a/gcc/ipa-split.c > +++ b/gcc/ipa-split.c > > Eventually we want ot teach ipa-split to do more adjustments, like > adding new > parameters for values computed in header that needs to be passed to tail > :) That is definitely possible with the new infrastructure. omp-simd-clone.c adds new parameters with it. The price is that enabling ipa_param_body_adjustments to work both as part the of call graph cloning machinery and also in a standalone mode makes it a bit bulky. > diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c > index 9bf1c4080f5..88895105bdc 100644 > --- a/gcc/tree-inline.c > +++ b/gcc/tree-inline.c > @@ -191,7 +190,17 @@ remap_ssa_name (tree name, copy_body_data *id) > > n = id->decl_map->get (name); > if (n) > - return unshare_expr (*n); > + { > + if (id->killed_new_ssa_names > + && id->killed_new_ssa_names->contains (*n)) > + { > + gcc_assert (processing_debug_stmt); > + processing_debug_stmt = -1; > + return name; > + } > + > + return unshare_expr (*n); > + } > > if (processing_debug_stmt) > { > @@ -1872,6 +1881,21 @@ remap_gimple_stmt (gimple *stmt, copy_body_data > *id) > gcc_assert (n); > gimple_set_block (copy, *n); > } > + if (id->param_body_adjs) > + { > + gimple_seq extra_stmts = NULL; > + id->param_body_adjs->modify_gimple_stmt (©, &extra_stmts); > + if (!gimple_seq_empty_p (extra_stmts)) > + { > + memset (&wi, 0, sizeof (wi)); > + wi.info = id; > + for (gimple_stmt_iterator egsi = gsi_start (extra_stmts); > + !gsi_end_p (egsi); > + gsi_next (&egsi)) > + walk_gimple_op (gsi_stmt (egsi), remap_gimple_op_r, &wi); > + gimple_seq_add_seq (&stmts, extra_stmts); > + } > + } This hunk just calls into param_body_adjs to actually perform IPA-SRA transformations on individual statements. it has nothing to do with killed_new_ssa_names. Sometimes a transformed statement ends up as a sequence of statements (simply because I need to create a conversion statement if there is a type mismatch) and then I have to re-map all of the resulting sequence and insert them into the new function. > @@ -2794,10 +2818,20 @@ redirect_all_calls (copy_body_data * id, > basic_block bb) > gimple *stmt = gsi_stmt (si); > if (is_gimple_call (stmt)) > { > + tree old_lhs = gimple_call_lhs (stmt); > struct cgraph_edge *edge = id->dst_node->get_edge (stmt); > if (edge) > { > edge->redirect_call_stmt_to_callee (); > + if (old_lhs > + && TREE_CODE (old_lhs) == SSA_NAME > + && !gimple_call_lhs (edge->call_stmt)) > + { > + if (!id->killed_new_ssa_names) > + id->killed_new_ssa_names = new hash_set<tree> (16); > + id->killed_new_ssa_names->add (old_lhs); > + } > + > > Some enlightening comments would help for those three hunks :) > So if LHS of call died you need to get rid of it. > I wonder if one can't also just store 0 to it and let later cleanups to > do their job? The dead SSAs only appeared in debug statements, they belong to return values which are otherwise unused. I'm afraid we'd then generate debug info claiming the value is zero (but I admit I have not tried). But I added comments in the two hunks manipulating killed_new_ssa_names. > @@ -5653,7 +5721,7 @@ copy_decl_for_dup_finish (copy_body_data *id, tree > decl, tree copy) > return copy; > } > > -static tree > +tree > copy_decl_to_var (tree decl, copy_body_data *id) > { > tree copy, type; > > Add a comment what function does, especially when it is exported now ;) OK, thanks, I'm looking forward to any further suggestions. Martin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] True IPA reimplementation of IPA-SRA 2019-05-10 8:31 [PATCH] True IPA reimplementation of IPA-SRA Martin Jambor 2019-05-16 12:09 ` Richard Biener @ 2019-06-18 12:00 ` Martin Liška 1 sibling, 0 replies; 7+ messages in thread From: Martin Liška @ 2019-06-18 12:00 UTC (permalink / raw) To: Martin Jambor, GCC Patches; +Cc: Jan Hubicka On 5/10/19 10:31 AM, Martin Jambor wrote: > Thanks in advance for any questions, comments and suggestions, Hi. I have just a small note that I would appreciate a dbgcnt for the future. I bet it will be useful for a test-case reduction. Thank you, Martin ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-26 14:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-10 8:31 [PATCH] True IPA reimplementation of IPA-SRA Martin Jambor 2019-05-16 12:09 ` Richard Biener 2019-05-16 14:04 ` Martin Jambor 2019-05-16 18:38 ` Richard Biener 2019-06-13 14:26 ` Jan Hubicka 2019-06-26 14:46 ` Martin Jambor 2019-06-18 12:00 ` Martin Liška
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).