On Thu, May 16, 2019 at 4:04 PM Martin Jambor <mjam...@suse.cz> wrote:
>
> Hi Richi,
>
> On Thu, May 16 2019, Richard Biener wrote:
> > On Fri, May 10, 2019 at 10:31 AM Martin Jambor <mjam...@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 <mjam...@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.
> >>
> >>
