[PATCH] True IPA reimplementation of IPA-SRA

[PATCH] True IPA reimplementation of IPA-SRA

[Martin Jambor]

[-- 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 --]

Re: [PATCH] True IPA reimplementation of IPA-SRA

[Richard Biener]

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.
>
>

Re: [PATCH] True IPA reimplementation of IPA-SRA

[Martin Jambor]

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.
>>
>>

Re: [PATCH] True IPA reimplementation of IPA-SRA

[Richard Biener]

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.
> >>
> >>

Re: [PATCH] True IPA reimplementation of IPA-SRA

[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 (&copy, &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 ;)

Re: [PATCH] True IPA reimplementation of IPA-SRA

[Martin Liška]

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

Re: [PATCH] True IPA reimplementation of IPA-SRA

[Martin Jambor]

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 (&copy, &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