Re: [PATCH 4/4] Data structure is used for inline_summary struct.
> > 2014-12-08 Martin Liska > > * lto-partition.c (add_symbol_to_partition_1): New inline_summary_d > is used. > (undo_partition): Likewise. > (lto_balanced_map): Likewise. > > gcc/ChangeLog: > > 2014-12-08 Martin Liska > > * cgraphunit.c (symbol_table::process_new_functions): New > inline_summary_d > is used. > * ipa-cp.c (ipcp_cloning_candidate_p): Likewise. > (devirtualization_time_bonus): Likewise. > (estimate_local_effects): Likewise. > (ipcp_propagate_stage): Likewise. > * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Likewise. > (evaluate_properties_for_edge): Likewise. > (inline_summary_alloc): Likewise. > (reset_inline_summary): New inline_summary argument is introduced. > (inline_summary_t::remove): New function. > (inline_summary_t::duplicate): Likewise. > (dump_inline_edge_summary): New inline_summary_d is used. > (dump_inline_summary): Likewise. > (estimate_function_body_sizes): Likewise. > (compute_inline_parameters): Likewise. > (estimate_edge_devirt_benefit): Likewise. > (estimate_node_size_and_time): Likewise. > (inline_update_callee_summaries): Likewise. > (inline_merge_summary): Likewise. > (inline_update_overall_summary): Likewise. > (simple_edge_hints): Likewise. > (do_estimate_edge_time): Likewise. > (estimate_time_after_inlining): Likewise. > (estimate_size_after_inlining): Likewise. > (do_estimate_growth): Likewise. > (growth_likely_positive): Likewise. > (inline_generate_summary): Likewise. > (inline_read_section): Likewise. > (inline_read_summary): Likewise. > (inline_write_summary): Likewise. > (inline_free_summary): Likewise. > * ipa-inline-transform.c (clone_inlined_nodes): Likewise. > (inline_call): Likewise. > * ipa-inline.c (caller_growth_limits): Likewise. > (can_inline_edge_p): Likewise. > (want_early_inline_function_p): Likewise. > (compute_uninlined_call_time): Likewise. > (compute_inlined_call_time): Likewise. > (big_speedup_p): Likewise. > (want_inline_small_function_p): Likewise. > (edge_badness): Likewise. > (update_caller_keys): Likewise. > (update_callee_keys): Likewise. > (recursive_inlining): Likewise. > (inline_small_functions): Likewise. > (inline_to_all_callers): Likewise. > (dump_overall_stats): Likewise. > (early_inline_small_functions): Likewise. > * ipa-inline.h: New class inline_summary_t replaces > vec. > * ipa-split.c (execute_split_functions): New inline_summary_d is used. > * ipa.c (walk_polymorphic_call_targets): Likewise. > * tree-sra.c (ipa_sra_preliminary_function_checks): Likewise. Patch is OK. I only changed mind about inline_summary_d. It does not look good used with inline_summary_d->get() especially because _d prefix was used in C++ication days for type names only. Probably inline_summaries->get() is better. OK with this change. Thanks, Honza
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On 11/14/2014 04:24 PM, Martin Liška wrote: On 11/14/2014 03:09 PM, Martin Liška wrote: On 11/13/2014 05:04 PM, Jan Hubicka wrote: + if (!inline_summary_summary) +inline_summary_summary = (inline_summary_cgraph_summary *) inline_summary_cgraph_summary::create_ggc (symtab); Hehe, this is funny naming scheme. Peraps inline_summary_d and inline_summary_t for the data and type? Hello. I adopted suggested naming scheme. - -static void -inline_node_duplication_hook (struct cgraph_node *src, - struct cgraph_node *dst, - ATTRIBUTE_UNUSED void *data) +void +inline_summary_cgraph_summary::duplication_hook (cgraph_node *src, + cgraph_node *dst, + inline_summary *, + inline_summary *info) Becuase those are no longer "hooks" but virtual function, I guess we could call them simply duplicate/insert/remove. Agree with the change. In a way I would like to see these to be methods of the underlying type rather than virtual methods of the summary, becuase these are operations on the data themselves. I was thinking to model these by specual constructor and copy constructor (taking the extra node pointer parameters) and standard destructor. I am not sure this would be more understandable this way? Motivation for this implementation is: a) it's useful to have an access to cgraph_node that is associated with a sumary b) with GTY, we cannot call destructors -/* Need a typedef for inline_summary because of inline function - 'inline_summary' below. */ -typedef struct inline_summary inline_summary_t; -extern GTY(()) vec *inline_summary_vec; +class GTY((user)) inline_summary_cgraph_summary: public cgraph_summary +{ +public: + inline_summary_cgraph_summary (symbol_table *symtab, bool ggc): +cgraph_summary (symtab, ggc) {} + + static inline_summary_cgraph_summary *create_ggc (symbol_table *symtab) + { +inline_summary_cgraph_summary *summary = new (ggc_cleared_alloc ()) inline_summary_cgraph_summary(symtab, true); +summary->disable_insertion_hook (); +return summary; + } + + + virtual void insertion_hook (cgraph_node *, inline_summary *); + virtual void removal_hook (cgraph_node *node, inline_summary *); + virtual void duplication_hook (cgraph_node *src, cgraph_node *dst, inline_summary *src_data, inline_summary *dst_data); +}; + +extern GTY(()) cgraph_summary *inline_summary_summary; All in all it looks better than original code. If we moved insert/ /* Information kept about parameter of call site. */ struct inline_param_summary @@ -249,10 +265,10 @@ void clone_inlined_nodes (struct cgraph_edge *e, bool, bool, int *, extern int ncalls_inlined; extern int nfunctions_inlined; -static inline struct inline_summary * -inline_summary (struct cgraph_node *node) +static inline inline_summary * +get_inline_summary (const struct cgraph_node *node) { - return &(*inline_summary_vec)[node->uid]; + return (*inline_summary_summary)[node->summary_uid]; Hmm, i guess there is no way to avoid the (*...)? Otherwise it would be cleaner to use inline_summary[...] instead of get_inline_summary IMO. I added function_summary::get method, where the usage looks cleaner: inline_summary_d->get (node). Thanks, Martin Thanks for working on this! Honza Patch v3. Martin Version v4. Martin >From 4f87fdf746532a9ad89fcf85246cc449a49ba09d Mon Sep 17 00:00:00 2001 From: mliska Date: Mon, 8 Dec 2014 11:33:08 +0100 Subject: [PATCH 3/3] symbol_summary is used for inline_summary. gcc/lto/ChangeLog: 2014-12-08 Martin Liska * lto-partition.c (add_symbol_to_partition_1): New inline_summary_d is used. (undo_partition): Likewise. (lto_balanced_map): Likewise. gcc/ChangeLog: 2014-12-08 Martin Liska * cgraphunit.c (symbol_table::process_new_functions): New inline_summary_d is used. * ipa-cp.c (ipcp_cloning_candidate_p): Likewise. (devirtualization_time_bonus): Likewise. (estimate_local_effects): Likewise. (ipcp_propagate_stage): Likewise. * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Likewise. (evaluate_properties_for_edge): Likewise. (inline_summary_alloc): Likewise. (reset_inline_summary): New inline_summary argument is introduced. (inline_summary_t::remove): New function. (inline_summary_t::duplicate): Likewise. (dump_inline_edge_summary): New inline_summary_d is used. (dump_inline_summary): Likewise. (estimate_function_body_sizes): Likewise. (compute_inline_parameters): Likewise. (estimate_edge_devirt_benefit): Likewise. (estimate_node_size_and_time): Likewise. (inline_update_callee_summaries): Likewise. (inline_merge_summary): Likewise. (inline_update_overall_summary): Likewise. (simple_edge_hints): Likewise. (do_estimate_edge_time): Likewise. (estimate_time_after_inlining): Likewise. (estimate_size_after_inlining): Likewise. (do_estimate_growth): Likewise. (growth_likely_positive): Likewise. (inline_generate_summary): Likewise. (inlin
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On 11/18/2014 11:25 PM, Martin Jambor wrote: On Tue, Nov 18, 2014 at 07:59:26PM +0100, Jan Hubicka wrote: Hi, On Tue, Nov 18, 2014 at 04:39:00PM +0100, Jan Hubicka wrote: On Fri, Nov 14, 2014 at 08:59:10PM +0100, Jan Hubicka wrote: b) with GTY, we cannot call destructor Everything in symbol table is expecitely memory managed (i.e. enver left to be freed by garbage collector). It resists in GTY only to allow linking garbage collected object from them and to get PCH working. Well, if I understand the intent correctly, summaries are for stuff that is not in the symbol table. For example jump functions are a Correct. vector of structures possibly containing trees, so everything has to be in garbage collected memory. When an edge is removed, it is necessary to be notified about it immediately, for example to decrement rdesc_refcount (you might argue that that should be done in a separate hook and not from within a summary class but then you start to rely on hook invocation ordering so I think it is better to eventually use the summaries for it too). I do not see why ctors/dtors can not do the reference counting. In fact this is how refcounting is done usually anyway? Well, when there is no garbage collection involved then yes, that is how you normally do it but in the GC case, there is the question of what is the appropriate time to call destructor on garbage collected data (like jump functions)? I still fail to see problem here. Summaries are explicitly managed- they are constructed at summary construction time or when new callgarph node is introduced/duplicated. They are destroyed when callgarph node is destroyed or whole summary is ddestroyed. It is job of the summary datastructure to call proper ctors/dtors, not job of garbage collector that provides the underlying memory management. I do not think that all summaries (in the meaning of a description of one particular symbol table node or call graph edge) are explicitely managed. For example ipa_edge_args or ipa_agg_replacement_value (which my alignment patch changes to ipcp_transformation_summary) are allocated in GC memory because they contain trees. If you have datastructure that points to something that is not explicitly managed (i.e. tree expression), you just can not have non-trivial constructor on that datastructure, because that is freed transparently by gty that don't do destruction... I admit to not being particularly bright today but that seems to be exactly my point. Well, in your case you have datastructure jump_function that contain a pointer to tree (EXPR). What I am trying to explain is that I see no reson why jump_function needs to be POD. I never said that the summary object needs to be a POD, I only said I liked the possibility of storing very simple objects (without wrapping them in classes with constructors and destructors). That is of course nothing more than my personal preference. The tree pointed to by EXPR pointer can not have a dtor by itself because GGC will not call it upon freeing. It is true that jump_function lives in GGC memory (to make pointer to expr work) but it never gets removed by ggc_collect because it is always pointed to by the summary datastructure. There are two ways to free the jump_function datastructure. 1) removing the symbol node it is attached to. Here the symtab code will call removal hook that was registered by container template. The container will call destructor of jump_function and the ggc_free its memory 2) removing the summary. In this case I would again expect the container template to walk all summaries and free them. So even if your structure lives in GGC memory it is not really garbage collected and thus the lack of machinery to call dtors at a time ggc decides to free something is not a problem? In fact looking at struct default_hashmap_traits, I see: /* Called to dispose of the key and value before marking the entry as deleted. */ template static void remove (T &v) { v.~T (); } Now I see, I should have read your previous email more carefully, by explicitely managed you mean that destructors will be called explicitely by the summary infrastructure. I was wondering how you wanted to rip the summaries out of GGC memory. Well, I suppose that would work, and since explicit calls to destructors are basically the counterpart of placement new that we already plan to use, it might be actually be the proper C++ thing to do. (I am not sure I like it though, for all other purposes the summary objects will look like managed by the garbage collector and only we who read this thread will know that the lifetime of the object would be decoupled from the allocation-span of its memory). Thanks for the clarification, Martin Hello. I tried to come up with ctor/dtor solution for types passes to symbol_summary template class. Example: struct inline_summary { inline_summary (cgraph_node *node); inline_summary (
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On Tue, Nov 18, 2014 at 07:59:26PM +0100, Jan Hubicka wrote: > > Hi, > > > > On Tue, Nov 18, 2014 at 04:39:00PM +0100, Jan Hubicka wrote: > > > > On Fri, Nov 14, 2014 at 08:59:10PM +0100, Jan Hubicka wrote: > > > > > > > > > > > > > > > b) with GTY, we cannot call destructor > > > > > > > > > > > > > > Everything in symbol table is expecitely memory managed (i.e. > > > > > > > enver left > > > > > > > to be freed by garbage collector). It resists in GTY only to > > > > > > > allow linking > > > > > > > garbage collected object from them and to get PCH working. > > > > > > > > > > > > > > > > > > > Well, if I understand the intent correctly, summaries are for stuff > > > > > > that is not in the symbol table. For example jump functions are a > > > > > Correct. > > > > > > vector of structures possibly containing trees, so everything has to > > > > > > be in garbage collected memory. > > > > > > > > > > > > When an edge is removed, it is necessary to be notified about it > > > > > > immediately, for example to decrement rdesc_refcount (you might > > > > > > argue > > > > > > that that should be done in a separate hook and not from within a > > > > > > summary class but then you start to rely on hook invocation ordering > > > > > > so I think it is better to eventually use the summaries for it too). > > > > > > > > > > I do not see why ctors/dtors can not do the reference counting. In > > > > > fact > > > > > this is how refcounting is done usually anyway? > > > > > > > > > > > > > Well, when there is no garbage collection involved then yes, that is > > > > how you normally do it but in the GC case, there is the question of > > > > what is the appropriate time to call destructor on garbage collected > > > > data (like jump functions)? > > > > > > I still fail to see problem here. Summaries are explicitly managed- they > > > are > > > constructed at summary construction time or when new callgarph node is > > > introduced/duplicated. They are destroyed when callgarph node is > > > destroyed or > > > whole summary is ddestroyed. It is job of the summary datastructure to > > > call > > > proper ctors/dtors, not job of garbage collector that provides the > > > underlying > > > memory management. > > > > I do not think that all summaries (in the meaning of a description of > > one particular symbol table node or call graph edge) are explicitely > > managed. For example ipa_edge_args or ipa_agg_replacement_value > > (which my alignment patch changes to ipcp_transformation_summary) are > > allocated in GC memory because they contain trees. > > > > > > > > If you have datastructure that points to something that is not > > > explicitly managed (i.e. tree expression), you just can not have > > > non-trivial constructor on that datastructure, because that is freed > > > transparently by gty that don't do destruction... > > > > I admit to not being particularly bright today but that seems to be > > exactly my point. > > Well, in your case you have datastructure jump_function that contain a pointer > to tree (EXPR). What I am trying to explain is that I see no reson why > jump_function needs to be POD. The tree pointed to by EXPR pointer can not > have a dtor by itself because GGC will not call it upon freeing. ggc does call dtors when it is about to sweep an object. however if you explicitly call ggc_free on an object with a dtor you need to call the dtor manually I believe. > It is true that jump_function lives in GGC memory (to make pointer to expr > work) but it never gets removed by ggc_collect because it is always pointed to > by the summary datastructure. There are two ways to free the jump_function > datastructure. assuming it doesn't need to go into pch, and I would really think it never does there's no real reason it has to live on the heap, you could iterate over all the summaries and mark all the trees they point at "manually". > 1) removing the symbol node it is attached to. > Here the symtab code will call removal hook that was registered by > container > template. The container will call destructor of jump_function and the > ggc_free > its memory > 2) removing the summary. In this case I would again expect the container > template to walk all summaries and free them. > > So even if your structure lives in GGC memory it is not really garbage > collected and thus the lack of machinery to call dtors at a time ggc decides > to > free something is not a problem? > > In fact looking at struct default_hashmap_traits, I see: > > /* Called to dispose of the key and value before marking the entry as > deleted. */ > > template static void remove (T &v) { v.~T (); } > > This trait gets called by the underlying hash table so if you have explicitly > managed hashmap (in GGC memory or not), things just work. Only catch is that > if you let your hashmap to be garbage collected, then your dtor is not called. actually I think it should be, ggc tra
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On Tue, Nov 18, 2014 at 07:59:26PM +0100, Jan Hubicka wrote: > > Hi, > > > > On Tue, Nov 18, 2014 at 04:39:00PM +0100, Jan Hubicka wrote: > > > > On Fri, Nov 14, 2014 at 08:59:10PM +0100, Jan Hubicka wrote: > > > > > > > > > > > > > > > b) with GTY, we cannot call destructor > > > > > > > > > > > > > > Everything in symbol table is expecitely memory managed (i.e. > > > > > > > enver left > > > > > > > to be freed by garbage collector). It resists in GTY only to > > > > > > > allow linking > > > > > > > garbage collected object from them and to get PCH working. > > > > > > > > > > > > > > > > > > > Well, if I understand the intent correctly, summaries are for stuff > > > > > > that is not in the symbol table. For example jump functions are a > > > > > Correct. > > > > > > vector of structures possibly containing trees, so everything has to > > > > > > be in garbage collected memory. > > > > > > > > > > > > When an edge is removed, it is necessary to be notified about it > > > > > > immediately, for example to decrement rdesc_refcount (you might > > > > > > argue > > > > > > that that should be done in a separate hook and not from within a > > > > > > summary class but then you start to rely on hook invocation ordering > > > > > > so I think it is better to eventually use the summaries for it too). > > > > > > > > > > I do not see why ctors/dtors can not do the reference counting. In > > > > > fact > > > > > this is how refcounting is done usually anyway? > > > > > > > > > > > > > Well, when there is no garbage collection involved then yes, that is > > > > how you normally do it but in the GC case, there is the question of > > > > what is the appropriate time to call destructor on garbage collected > > > > data (like jump functions)? > > > > > > I still fail to see problem here. Summaries are explicitly managed- they > > > are > > > constructed at summary construction time or when new callgarph node is > > > introduced/duplicated. They are destroyed when callgarph node is > > > destroyed or > > > whole summary is ddestroyed. It is job of the summary datastructure to > > > call > > > proper ctors/dtors, not job of garbage collector that provides the > > > underlying > > > memory management. > > > > I do not think that all summaries (in the meaning of a description of > > one particular symbol table node or call graph edge) are explicitely > > managed. For example ipa_edge_args or ipa_agg_replacement_value > > (which my alignment patch changes to ipcp_transformation_summary) are > > allocated in GC memory because they contain trees. > > > > > > > > If you have datastructure that points to something that is not > > > explicitly managed (i.e. tree expression), you just can not have > > > non-trivial constructor on that datastructure, because that is freed > > > transparently by gty that don't do destruction... > > > > I admit to not being particularly bright today but that seems to be > > exactly my point. > > Well, in your case you have datastructure jump_function that contain a pointer > to tree (EXPR). What I am trying to explain is that I see no reson why > jump_function needs to be POD. I never said that the summary object needs to be a POD, I only said I liked the possibility of storing very simple objects (without wrapping them in classes with constructors and destructors). That is of course nothing more than my personal preference. > The tree pointed to by EXPR pointer can not > have a dtor by itself because GGC will not call it upon freeing. > > It is true that jump_function lives in GGC memory (to make pointer to expr > work) but it never gets removed by ggc_collect because it is always pointed to > by the summary datastructure. There are two ways to free the jump_function > datastructure. > 1) removing the symbol node it is attached to. > Here the symtab code will call removal hook that was registered by > container > template. The container will call destructor of jump_function and the > ggc_free > its memory > 2) removing the summary. In this case I would again expect the container > template to walk all summaries and free them. > > So even if your structure lives in GGC memory it is not really garbage > collected and thus the lack of machinery to call dtors at a time ggc decides > to > free something is not a problem? > > In fact looking at struct default_hashmap_traits, I see: > > /* Called to dispose of the key and value before marking the entry as > deleted. */ > > template static void remove (T &v) { v.~T (); } Now I see, I should have read your previous email more carefully, by explicitely managed you mean that destructors will be called explicitely by the summary infrastructure. I was wondering how you wanted to rip the summaries out of GGC memory. Well, I suppose that would work, and since explicit calls to destructors are basically the counterpart of placement new that we already plan to use, it might be actually
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
> Hi, > > On Tue, Nov 18, 2014 at 04:39:00PM +0100, Jan Hubicka wrote: > > > On Fri, Nov 14, 2014 at 08:59:10PM +0100, Jan Hubicka wrote: > > > > > > > > > > > > > b) with GTY, we cannot call destructor > > > > > > > > > > > > Everything in symbol table is expecitely memory managed (i.e. enver > > > > > > left > > > > > > to be freed by garbage collector). It resists in GTY only to allow > > > > > > linking > > > > > > garbage collected object from them and to get PCH working. > > > > > > > > > > > > > > > > Well, if I understand the intent correctly, summaries are for stuff > > > > > that is not in the symbol table. For example jump functions are a > > > > Correct. > > > > > vector of structures possibly containing trees, so everything has to > > > > > be in garbage collected memory. > > > > > > > > > > When an edge is removed, it is necessary to be notified about it > > > > > immediately, for example to decrement rdesc_refcount (you might argue > > > > > that that should be done in a separate hook and not from within a > > > > > summary class but then you start to rely on hook invocation ordering > > > > > so I think it is better to eventually use the summaries for it too). > > > > > > > > I do not see why ctors/dtors can not do the reference counting. In fact > > > > this is how refcounting is done usually anyway? > > > > > > > > > > Well, when there is no garbage collection involved then yes, that is > > > how you normally do it but in the GC case, there is the question of > > > what is the appropriate time to call destructor on garbage collected > > > data (like jump functions)? > > > > I still fail to see problem here. Summaries are explicitly managed- they > > are > > constructed at summary construction time or when new callgarph node is > > introduced/duplicated. They are destroyed when callgarph node is destroyed > > or > > whole summary is ddestroyed. It is job of the summary datastructure to call > > proper ctors/dtors, not job of garbage collector that provides the > > underlying > > memory management. > > I do not think that all summaries (in the meaning of a description of > one particular symbol table node or call graph edge) are explicitely > managed. For example ipa_edge_args or ipa_agg_replacement_value > (which my alignment patch changes to ipcp_transformation_summary) are > allocated in GC memory because they contain trees. > > > > > If you have datastructure that points to something that is not > > explicitly managed (i.e. tree expression), you just can not have > > non-trivial constructor on that datastructure, because that is freed > > transparently by gty that don't do destruction... > > I admit to not being particularly bright today but that seems to be > exactly my point. Well, in your case you have datastructure jump_function that contain a pointer to tree (EXPR). What I am trying to explain is that I see no reson why jump_function needs to be POD. The tree pointed to by EXPR pointer can not have a dtor by itself because GGC will not call it upon freeing. It is true that jump_function lives in GGC memory (to make pointer to expr work) but it never gets removed by ggc_collect because it is always pointed to by the summary datastructure. There are two ways to free the jump_function datastructure. 1) removing the symbol node it is attached to. Here the symtab code will call removal hook that was registered by container template. The container will call destructor of jump_function and the ggc_free its memory 2) removing the summary. In this case I would again expect the container template to walk all summaries and free them. So even if your structure lives in GGC memory it is not really garbage collected and thus the lack of machinery to call dtors at a time ggc decides to free something is not a problem? In fact looking at struct default_hashmap_traits, I see: /* Called to dispose of the key and value before marking the entry as deleted. */ template static void remove (T &v) { v.~T (); } This trait gets called by the underlying hash table so if you have explicitly managed hashmap (in GGC memory or not), things just work. Only catch is that if you let your hashmap to be garbage collected, then your dtor is not called. So probably the dtors are working same way with Martin's summaries? I guess we can follow same scheme here, have summary_traits that default to calling correspondin ctors/dtors. Honza > > Martin
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
Hi, On Tue, Nov 18, 2014 at 04:39:00PM +0100, Jan Hubicka wrote: > > On Fri, Nov 14, 2014 at 08:59:10PM +0100, Jan Hubicka wrote: > > > > > > > > > > > b) with GTY, we cannot call destructor > > > > > > > > > > Everything in symbol table is expecitely memory managed (i.e. enver > > > > > left > > > > > to be freed by garbage collector). It resists in GTY only to allow > > > > > linking > > > > > garbage collected object from them and to get PCH working. > > > > > > > > > > > > > Well, if I understand the intent correctly, summaries are for stuff > > > > that is not in the symbol table. For example jump functions are a > > > Correct. > > > > vector of structures possibly containing trees, so everything has to > > > > be in garbage collected memory. > > > > > > > > When an edge is removed, it is necessary to be notified about it > > > > immediately, for example to decrement rdesc_refcount (you might argue > > > > that that should be done in a separate hook and not from within a > > > > summary class but then you start to rely on hook invocation ordering > > > > so I think it is better to eventually use the summaries for it too). > > > > > > I do not see why ctors/dtors can not do the reference counting. In fact > > > this is how refcounting is done usually anyway? > > > > > > > Well, when there is no garbage collection involved then yes, that is > > how you normally do it but in the GC case, there is the question of > > what is the appropriate time to call destructor on garbage collected > > data (like jump functions)? > > I still fail to see problem here. Summaries are explicitly managed- they are > constructed at summary construction time or when new callgarph node is > introduced/duplicated. They are destroyed when callgarph node is destroyed or > whole summary is ddestroyed. It is job of the summary datastructure to call > proper ctors/dtors, not job of garbage collector that provides the underlying > memory management. I do not think that all summaries (in the meaning of a description of one particular symbol table node or call graph edge) are explicitely managed. For example ipa_edge_args or ipa_agg_replacement_value (which my alignment patch changes to ipcp_transformation_summary) are allocated in GC memory because they contain trees. > > If you have datastructure that points to something that is not > explicitly managed (i.e. tree expression), you just can not have > non-trivial constructor on that datastructure, because that is freed > transparently by gty that don't do destruction... I admit to not being particularly bright today but that seems to be exactly my point. Martin
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
> On Fri, Nov 14, 2014 at 08:59:10PM +0100, Jan Hubicka wrote: > > > > > > > > > b) with GTY, we cannot call destructor > > > > > > > > Everything in symbol table is expecitely memory managed (i.e. enver left > > > > to be freed by garbage collector). It resists in GTY only to allow > > > > linking > > > > garbage collected object from them and to get PCH working. > > > > > > > > > > Well, if I understand the intent correctly, summaries are for stuff > > > that is not in the symbol table. For example jump functions are a > > Correct. > > > vector of structures possibly containing trees, so everything has to > > > be in garbage collected memory. > > > > > > When an edge is removed, it is necessary to be notified about it > > > immediately, for example to decrement rdesc_refcount (you might argue > > > that that should be done in a separate hook and not from within a > > > summary class but then you start to rely on hook invocation ordering > > > so I think it is better to eventually use the summaries for it too). > > > > I do not see why ctors/dtors can not do the reference counting. In fact > > this is how refcounting is done usually anyway? > > > > Well, when there is no garbage collection involved then yes, that is > how you normally do it but in the GC case, there is the question of > what is the appropriate time to call destructor on garbage collected > data (like jump functions)? I still fail to see problem here. Summaries are explicitly managed- they are constructed at summary construction time or when new callgarph node is introduced/duplicated. They are destroyed when callgarph node is destroyed or whole summary is ddestroyed. It is job of the summary datastructure to call proper ctors/dtors, not job of garbage collector that provides the underlying memory management. If you have datastructure that points to something that is not explicitly managed (i.e. tree expression), you just can not have non-trivial constructor on that datastructure, because that is freed transparently by gty that don't do destruction... Honza
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On Fri, Nov 14, 2014 at 08:59:10PM +0100, Jan Hubicka wrote: > > > > > > > b) with GTY, we cannot call destructor > > > > > > Everything in symbol table is expecitely memory managed (i.e. enver left > > > to be freed by garbage collector). It resists in GTY only to allow linking > > > garbage collected object from them and to get PCH working. > > > > > > > Well, if I understand the intent correctly, summaries are for stuff > > that is not in the symbol table. For example jump functions are a > Correct. > > vector of structures possibly containing trees, so everything has to > > be in garbage collected memory. > > > > When an edge is removed, it is necessary to be notified about it > > immediately, for example to decrement rdesc_refcount (you might argue > > that that should be done in a separate hook and not from within a > > summary class but then you start to rely on hook invocation ordering > > so I think it is better to eventually use the summaries for it too). > > I do not see why ctors/dtors can not do the reference counting. In fact > this is how refcounting is done usually anyway? > Well, when there is no garbage collection involved then yes, that is how you normally do it but in the GC case, there is the question of what is the appropriate time to call destructor on garbage collected data (like jump functions)? Martin
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On 11/14/2014 05:06 PM, Jan Hubicka wrote: In a way I would like to see these to be methods of the underlying type rather than virtual methods of the summary, becuase these are operations on the data themselves. I was thinking to model these by specual constructor and copy constructor (taking the extra node pointer parameters) and standard destructor. I am not sure this would be more understandable this way? Motivation for this implementation is: a) it's useful to have an access to cgraph_node that is associated with a sumary Yep, one would have node addition ctor (symtab_node *); (or cgraph/varpool nodes for cgraph/varpool annotations) that would default to ctor for implementations that do not care about node. And node duplication ctor ctor (summary &, symtab_node *, symtab_node *) that would default to copy constructor for data that do not need to be copied. Hello. I have no problem with such construction and destruction, we can also provide base implementation. I would say that main advantage (in addition to have a way to provide resonable defaults) is to make ctors/dtors of the embedded classes working well, so one can for example embedd pointer_map and not care about its construction/destruction. b) with GTY, we cannot call destructor Everything in symbol table is expecitely memory managed (i.e. enver left to be freed by garbage collector). It resists in GTY only to allow linking garbage collected object from them and to get PCH working. However GTY types need to be allocated by ggc_alloc and one can't call dtor. This was main motivation for providing hooks instead of ctor/dtor API. Maybe I miss something? Thanks, Martin This is however quite cosmetic issue I would preffer our C++ guys to comment on. We can tweak this incrementally. +void +inline_summary_t::duplicate (cgraph_node *src, +cgraph_node *dst, +inline_summary *, +inline_summary *info) Also we should have a way to say that the annotation do not need to be duplicated (for example when we do not want to annotate inline clones). Probably by adding duplicate_p predicate that is called before the actual duplication happens? The updated patch is OK, I will take a look on the main patch. Honza { - struct inline_summary *info; inline_summary_alloc (); - info = inline_summary (dst); - memcpy (info, inline_summary (src), sizeof (struct inline_summary)); + memcpy (info, inline_summary_d->get (src), sizeof (inline_summary)); /* TODO: as an optimization, we may avoid copying conditions that are known to be false or true. */ info->conds = vec_safe_copy (info->conds); @@ -1328,7 +1309,7 @@ free_growth_caches (void) static void dump_inline_edge_summary (FILE *f, int indent, struct cgraph_node *node, - struct inline_summary *info) + inline_summary *info) { struct cgraph_edge *edge; for (edge = node->callees; edge; edge = edge->next_callee) @@ -1345,8 +1326,8 @@ dump_inline_edge_summary (FILE *f, int indent, struct cgraph_node *node, ? "inlined" : cgraph_inline_failed_string (edge-> inline_failed), indent, "", es->loop_depth, edge->frequency, es->call_stmt_size, es->call_stmt_time, - (int) inline_summary (callee)->size / INLINE_SIZE_SCALE, - (int) inline_summary (callee)->estimated_stack_size); + (int) inline_summary_d->get (callee)->size / INLINE_SIZE_SCALE, + (int) inline_summary_d->get (callee)->estimated_stack_size); if (es->predicate) { @@ -1372,9 +1353,9 @@ dump_inline_edge_summary (FILE *f, int indent, struct cgraph_node *node, fprintf (f, "%*sStack frame offset %i, callee self size %i," " callee size %i\n", indent + 2, "", - (int) inline_summary (callee)->stack_frame_offset, - (int) inline_summary (callee)->estimated_self_stack_size, - (int) inline_summary (callee)->estimated_stack_size); + (int) inline_summary_d->get (callee)->stack_frame_offset, + (int) inline_summary_d->get (callee)->estimated_self_stack_size, + (int) inline_summary_d->get (callee)->estimated_stack_size); dump_inline_edge_summary (f, indent + 2, callee, info); } } @@ -1402,7 +1383,7 @@ dump_inline_summary (FILE *f, struct cgraph_node *node) { if (node->definition) { - struct inline_summary *s = inline_summary (node); + inline_summary *s = inline_summary_d->get (node); size_time_entry *e; int i; fprintf (f, "Inline summary for %s/%i", node->name (), @@ -1725,7 +1706,7 @@ eliminated_by_inlining_prob (gimple stmt) static void set_cond_stmt_execution_predicate (struct ipa_node_params *info, - struct inline_s
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
> > I would say that main advantage (in addition to have a way to provide > > resonable > > defaults) is to make ctors/dtors of the embedded classes working well, so > > one can > > for example embedd pointer_map and not care about its > > construction/destruction. > > It was actually me who suggested virtual methods instead of > constructors. My idea was to make the summaries very light-weight and > usable even for simple types. For example, during IPA-CP propagation > phase, I use an (edge) summary that is basically only a pointer to > cgraph_edge. I'd even prefer that the default duplication hook would > just do an assignment or a memcpy. Yep, I would hope there is a vairant that makes simple types easy but still allows to put non-PODs into the summaries (or is that possible with current code?) Perhaps we can have a base class that provides default implementation of the insertion/duplication ctor via the default ctor/copy ctor? That would still require to wrap your pointer in a trivial class, but you would not need to write any methods. The container is template anyway, so one would not need virtual methods for the maintainance? > > > > > > b) with GTY, we cannot call destructor > > > > Everything in symbol table is expecitely memory managed (i.e. enver left > > to be freed by garbage collector). It resists in GTY only to allow linking > > garbage collected object from them and to get PCH working. > > > > Well, if I understand the intent correctly, summaries are for stuff > that is not in the symbol table. For example jump functions are a Correct. > vector of structures possibly containing trees, so everything has to > be in garbage collected memory. > > When an edge is removed, it is necessary to be notified about it > immediately, for example to decrement rdesc_refcount (you might argue > that that should be done in a separate hook and not from within a > summary class but then you start to rely on hook invocation ordering > so I think it is better to eventually use the summaries for it too). I do not see why ctors/dtors can not do the reference counting. In fact this is how refcounting is done usually anyway? Well, I will read the actual implementation and of our C++ guys are fine with the current form, I think we can give it a try. Refining the "hooks" part of the API incrementally should be easy. Honza > > Thanks, > > Martin
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On Fri, Nov 14, 2014 at 05:06:44PM +0100, Jan Hubicka wrote: > > > > > >In a way I would like to see these to be methods of the underlying type > > >rather than > > >virtual methods of the summary, becuase these are operations on the data > > >themselves. > > >I was thinking to model these by specual constructor and copy constructor > > >(taking the extra node pointer parameters) and standard destructor. I am > > >not sure this > > >would be more understandable this way? > > > > Motivation for this implementation is: > > a) it's useful to have an access to cgraph_node that is associated with a > > sumary > > Yep, one would have node addition > ctor (symtab_node *); (or cgraph/varpool nodes for cgraph/varpool > annotations) > that would default to ctor for implementations that do not care about node. > And node duplication ctor > ctor (summary &, symtab_node *, symtab_node *) > that would default to copy constructor for data that do not need to be copied. > > I would say that main advantage (in addition to have a way to provide > resonable > defaults) is to make ctors/dtors of the embedded classes working well, so one > can > for example embedd pointer_map and not care about its > construction/destruction. It was actually me who suggested virtual methods instead of constructors. My idea was to make the summaries very light-weight and usable even for simple types. For example, during IPA-CP propagation phase, I use an (edge) summary that is basically only a pointer to cgraph_edge. I'd even prefer that the default duplication hook would just do an assignment or a memcpy. > > > b) with GTY, we cannot call destructor > > Everything in symbol table is expecitely memory managed (i.e. enver left > to be freed by garbage collector). It resists in GTY only to allow linking > garbage collected object from them and to get PCH working. > Well, if I understand the intent correctly, summaries are for stuff that is not in the symbol table. For example jump functions are a vector of structures possibly containing trees, so everything has to be in garbage collected memory. When an edge is removed, it is necessary to be notified about it immediately, for example to decrement rdesc_refcount (you might argue that that should be done in a separate hook and not from within a summary class but then you start to rely on hook invocation ordering so I think it is better to eventually use the summaries for it too). Thanks, Martin
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
> > > >In a way I would like to see these to be methods of the underlying type > >rather than > >virtual methods of the summary, becuase these are operations on the data > >themselves. > >I was thinking to model these by specual constructor and copy constructor > >(taking the extra node pointer parameters) and standard destructor. I am > >not sure this > >would be more understandable this way? > > Motivation for this implementation is: > a) it's useful to have an access to cgraph_node that is associated with a > sumary Yep, one would have node addition ctor (symtab_node *); (or cgraph/varpool nodes for cgraph/varpool annotations) that would default to ctor for implementations that do not care about node. And node duplication ctor ctor (summary &, symtab_node *, symtab_node *) that would default to copy constructor for data that do not need to be copied. I would say that main advantage (in addition to have a way to provide resonable defaults) is to make ctors/dtors of the embedded classes working well, so one can for example embedd pointer_map and not care about its construction/destruction. > b) with GTY, we cannot call destructor Everything in symbol table is expecitely memory managed (i.e. enver left to be freed by garbage collector). It resists in GTY only to allow linking garbage collected object from them and to get PCH working. This is however quite cosmetic issue I would preffer our C++ guys to comment on. We can tweak this incrementally. > +void > +inline_summary_t::duplicate (cgraph_node *src, > + cgraph_node *dst, > + inline_summary *, > + inline_summary *info) Also we should have a way to say that the annotation do not need to be duplicated (for example when we do not want to annotate inline clones). Probably by adding duplicate_p predicate that is called before the actual duplication happens? The updated patch is OK, I will take a look on the main patch. Honza > { > - struct inline_summary *info; >inline_summary_alloc (); > - info = inline_summary (dst); > - memcpy (info, inline_summary (src), sizeof (struct inline_summary)); > + memcpy (info, inline_summary_d->get (src), sizeof (inline_summary)); >/* TODO: as an optimization, we may avoid copying conditions > that are known to be false or true. */ >info->conds = vec_safe_copy (info->conds); > @@ -1328,7 +1309,7 @@ free_growth_caches (void) > > static void > dump_inline_edge_summary (FILE *f, int indent, struct cgraph_node *node, > - struct inline_summary *info) > + inline_summary *info) > { >struct cgraph_edge *edge; >for (edge = node->callees; edge; edge = edge->next_callee) > @@ -1345,8 +1326,8 @@ dump_inline_edge_summary (FILE *f, int indent, struct > cgraph_node *node, > ? "inlined" : cgraph_inline_failed_string (edge-> inline_failed), > indent, "", es->loop_depth, edge->frequency, > es->call_stmt_size, es->call_stmt_time, > -(int) inline_summary (callee)->size / INLINE_SIZE_SCALE, > -(int) inline_summary (callee)->estimated_stack_size); > +(int) inline_summary_d->get (callee)->size / INLINE_SIZE_SCALE, > +(int) inline_summary_d->get (callee)->estimated_stack_size); > >if (es->predicate) > { > @@ -1372,9 +1353,9 @@ dump_inline_edge_summary (FILE *f, int indent, struct > cgraph_node *node, > fprintf (f, "%*sStack frame offset %i, callee self size %i," > " callee size %i\n", > indent + 2, "", > -(int) inline_summary (callee)->stack_frame_offset, > -(int) inline_summary (callee)->estimated_self_stack_size, > -(int) inline_summary (callee)->estimated_stack_size); > +(int) inline_summary_d->get (callee)->stack_frame_offset, > +(int) inline_summary_d->get > (callee)->estimated_self_stack_size, > +(int) inline_summary_d->get (callee)->estimated_stack_size); > dump_inline_edge_summary (f, indent + 2, callee, info); > } > } > @@ -1402,7 +1383,7 @@ dump_inline_summary (FILE *f, struct cgraph_node *node) > { >if (node->definition) > { > - struct inline_summary *s = inline_summary (node); > + inline_summary *s = inline_summary_d->get (node); >size_time_entry *e; >int i; >fprintf (f, "Inline summary for %s/%i", node->name (), > @@ -1725,7 +1706,7 @@ eliminated_by_inlining_prob (gimple stmt) > > static void > set_cond_stmt_execution_predicate (struct ipa_node_params *info, > -struct inline_summary *summary, > +inline_summary *summary, > basic_block bb) > { >gimple last; > @@ -1810,7 +1791,7 @@ set_cond_stmt_execution_predicate (struct > ipa_node_params *info, > > static
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On 11/14/2014 03:09 PM, Martin Liška wrote: On 11/13/2014 05:04 PM, Jan Hubicka wrote: + if (!inline_summary_summary) +inline_summary_summary = (inline_summary_cgraph_summary *) inline_summary_cgraph_summary::create_ggc (symtab); Hehe, this is funny naming scheme. Peraps inline_summary_d and inline_summary_t for the data and type? Hello. I adopted suggested naming scheme. - -static void -inline_node_duplication_hook (struct cgraph_node *src, - struct cgraph_node *dst, - ATTRIBUTE_UNUSED void *data) +void +inline_summary_cgraph_summary::duplication_hook (cgraph_node *src, + cgraph_node *dst, + inline_summary *, + inline_summary *info) Becuase those are no longer "hooks" but virtual function, I guess we could call them simply duplicate/insert/remove. Agree with the change. In a way I would like to see these to be methods of the underlying type rather than virtual methods of the summary, becuase these are operations on the data themselves. I was thinking to model these by specual constructor and copy constructor (taking the extra node pointer parameters) and standard destructor. I am not sure this would be more understandable this way? Motivation for this implementation is: a) it's useful to have an access to cgraph_node that is associated with a sumary b) with GTY, we cannot call destructors -/* Need a typedef for inline_summary because of inline function - 'inline_summary' below. */ -typedef struct inline_summary inline_summary_t; -extern GTY(()) vec *inline_summary_vec; +class GTY((user)) inline_summary_cgraph_summary: public cgraph_summary +{ +public: + inline_summary_cgraph_summary (symbol_table *symtab, bool ggc): +cgraph_summary (symtab, ggc) {} + + static inline_summary_cgraph_summary *create_ggc (symbol_table *symtab) + { +inline_summary_cgraph_summary *summary = new (ggc_cleared_alloc ()) inline_summary_cgraph_summary(symtab, true); +summary->disable_insertion_hook (); +return summary; + } + + + virtual void insertion_hook (cgraph_node *, inline_summary *); + virtual void removal_hook (cgraph_node *node, inline_summary *); + virtual void duplication_hook (cgraph_node *src, cgraph_node *dst, inline_summary *src_data, inline_summary *dst_data); +}; + +extern GTY(()) cgraph_summary *inline_summary_summary; All in all it looks better than original code. If we moved insert/ /* Information kept about parameter of call site. */ struct inline_param_summary @@ -249,10 +265,10 @@ void clone_inlined_nodes (struct cgraph_edge *e, bool, bool, int *, extern int ncalls_inlined; extern int nfunctions_inlined; -static inline struct inline_summary * -inline_summary (struct cgraph_node *node) +static inline inline_summary * +get_inline_summary (const struct cgraph_node *node) { - return &(*inline_summary_vec)[node->uid]; + return (*inline_summary_summary)[node->summary_uid]; Hmm, i guess there is no way to avoid the (*...)? Otherwise it would be cleaner to use inline_summary[...] instead of get_inline_summary IMO. I added function_summary::get method, where the usage looks cleaner: inline_summary_d->get (node). Thanks, Martin Thanks for working on this! Honza Patch v3. Martin >From 7f57a3a762fecea9a20e307f06e868a73da98000 Mon Sep 17 00:00:00 2001 From: mliska Date: Fri, 14 Nov 2014 14:54:12 +0100 Subject: [PATCH 3/3] Data structure is used for inline_summary struct. gcc/ChangeLog: 2014-11-12 Martin Liska * cgraphunit.c (symbol_table::process_new_functions): inline_summary_vec is replaced with inline_summary_t. * ipa-cp.c (ipcp_cloning_candidate_p): Usage of inline_summary_t::get. (devirtualization_time_bonus): Likewise. (estimate_local_effects): Likewise. (ipcp_propagate_stage): Likewise. * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Likewise. (evaluate_properties_for_edge): Likewise. (inline_summary_alloc): Deletion of old hook holders. (reset_inline_summary): inline_summary is added as argument. (inline_summary_cgraph_summary::removal_hook): New function. (inline_summary_cgraph_summary::duplication_hook): Likewise. (dump_inline_edge_summary): Struct keyword removed. (dump_inline_summary): Likewise. (estimate_function_body_sizes): Usage of inline_summary_t::get. (compute_inline_parameters): Likewise. (estimate_edge_devirt_benefit): Struct keyword removed. (estimate_node_size_and_time): Likewise. (inline_update_callee_summaries): Likewise. (inline_merge_summary): Usage of inline_summary_t::get. (inline_update_overall_summary): Likewise. (simple_edge_hints): Likewise. (do_estimate_edge_time): Likewise. (estimate_time_after_inlining): Likewise. (estimate_size_after_inlining): Likewise. (do_estimate_growth): Likewise. (growth_likely_positive): Likewise. (inline_generate_summary): inline_summary_t is registered. (inline_read_section): Struct keyword removed. (inline_read_summary): Likewise.
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
On 11/13/2014 05:04 PM, Jan Hubicka wrote: + if (!inline_summary_summary) +inline_summary_summary = (inline_summary_cgraph_summary *) inline_summary_cgraph_summary::create_ggc (symtab); Hehe, this is funny naming scheme. Peraps inline_summary_d and inline_summary_t for the data and type? Hello. I adopted suggested naming scheme. - -static void -inline_node_duplication_hook (struct cgraph_node *src, - struct cgraph_node *dst, - ATTRIBUTE_UNUSED void *data) +void +inline_summary_cgraph_summary::duplication_hook (cgraph_node *src, + cgraph_node *dst, + inline_summary *, + inline_summary *info) Becuase those are no longer "hooks" but virtual function, I guess we could call them simply duplicate/insert/remove. Agree with the change. In a way I would like to see these to be methods of the underlying type rather than virtual methods of the summary, becuase these are operations on the data themselves. I was thinking to model these by specual constructor and copy constructor (taking the extra node pointer parameters) and standard destructor. I am not sure this would be more understandable this way? Motivation for this implementation is: a) it's useful to have an access to cgraph_node that is associated with a sumary b) with GTY, we cannot call destructors -/* Need a typedef for inline_summary because of inline function - 'inline_summary' below. */ -typedef struct inline_summary inline_summary_t; -extern GTY(()) vec *inline_summary_vec; +class GTY((user)) inline_summary_cgraph_summary: public cgraph_summary +{ +public: + inline_summary_cgraph_summary (symbol_table *symtab, bool ggc): +cgraph_summary (symtab, ggc) {} + + static inline_summary_cgraph_summary *create_ggc (symbol_table *symtab) + { +inline_summary_cgraph_summary *summary = new (ggc_cleared_alloc ()) inline_summary_cgraph_summary(symtab, true); +summary->disable_insertion_hook (); +return summary; + } + + + virtual void insertion_hook (cgraph_node *, inline_summary *); + virtual void removal_hook (cgraph_node *node, inline_summary *); + virtual void duplication_hook (cgraph_node *src, cgraph_node *dst, inline_summary *src_data, inline_summary *dst_data); +}; + +extern GTY(()) cgraph_summary *inline_summary_summary; All in all it looks better than original code. If we moved insert/ /* Information kept about parameter of call site. */ struct inline_param_summary @@ -249,10 +265,10 @@ void clone_inlined_nodes (struct cgraph_edge *e, bool, bool, int *, extern int ncalls_inlined; extern int nfunctions_inlined; -static inline struct inline_summary * -inline_summary (struct cgraph_node *node) +static inline inline_summary * +get_inline_summary (const struct cgraph_node *node) { - return &(*inline_summary_vec)[node->uid]; + return (*inline_summary_summary)[node->summary_uid]; Hmm, i guess there is no way to avoid the (*...)? Otherwise it would be cleaner to use inline_summary[...] instead of get_inline_summary IMO. I added function_summary::get method, where the usage looks cleaner: inline_summary_d->get (node). Thanks, Martin Thanks for working on this! Honza >From 6e8531d8d3659524e337c7c1d96596952c3ff0e8 Mon Sep 17 00:00:00 2001 From: mliska Date: Fri, 14 Nov 2014 14:54:12 +0100 Subject: [PATCH 3/3] Data structure is used for inline_summary struct. gcc/ChangeLog: 2014-11-12 Martin Liska * cgraphunit.c (symbol_table::process_new_functions): inline_summary_vec is replaced with inline_summary_t. * ipa-cp.c (ipcp_cloning_candidate_p): Usage of inline_summary_d::get. (devirtualization_time_bonus): Likewise. (estimate_local_effects): Likewise. (ipcp_propagate_stage): Likewise. * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Likewise. (evaluate_properties_for_edge): Likewise. (inline_summary_alloc): Deletion of old hook holders. (reset_inline_summary): inline_summary is added as argument. (inline_summary_cgraph_summary::removal_hook): New function. (inline_summary_cgraph_summary::duplication_hook): Likewise. (dump_inline_edge_summary): Struct keyword removed. (dump_inline_summary): Likewise. (estimate_function_body_sizes): Usage of inline_summary_d::get. (compute_inline_parameters): Likewise. (estimate_edge_devirt_benefit): Struct keyword removed. (estimate_node_size_and_time): Likewise. (inline_update_callee_summaries): Likewise. (inline_merge_summary): Usage of inline_summary_d::get. (inline_update_overall_summary): Likewise. (simple_edge_hints): Likewise. (do_estimate_edge_time): Likewise. (estimate_time_after_inlining): Likewise. (estimate_size_after_inlining): Likewise. (do_estimate_growth): Likewise. (growth_likely_positive): Likewise. (inline_generate_summary): inline_summary_t is registered. (inline_read_section): Struct keyword removed. (inline_read_summary): Likewise. (inline
Re: [PATCH 4/4] Data structure is used for inline_summary struct.
> + if (!inline_summary_summary) > +inline_summary_summary = (inline_summary_cgraph_summary *) > inline_summary_cgraph_summary::create_ggc (symtab); Hehe, this is funny naming scheme. Peraps inline_summary_d and inline_summary_t for the data and type? > - > -static void > -inline_node_duplication_hook (struct cgraph_node *src, > - struct cgraph_node *dst, > - ATTRIBUTE_UNUSED void *data) > +void > +inline_summary_cgraph_summary::duplication_hook (cgraph_node *src, > + cgraph_node *dst, > + inline_summary *, > + inline_summary *info) Becuase those are no longer "hooks" but virtual function, I guess we could call them simply duplicate/insert/remove. In a way I would like to see these to be methods of the underlying type rather than virtual methods of the summary, becuase these are operations on the data themselves. I was thinking to model these by specual constructor and copy constructor (taking the extra node pointer parameters) and standard destructor. I am not sure this would be more understandable this way? > -/* Need a typedef for inline_summary because of inline function > - 'inline_summary' below. */ > -typedef struct inline_summary inline_summary_t; > -extern GTY(()) vec *inline_summary_vec; > +class GTY((user)) inline_summary_cgraph_summary: public cgraph_summary > > +{ > +public: > + inline_summary_cgraph_summary (symbol_table *symtab, bool ggc): > +cgraph_summary (symtab, ggc) {} > + > + static inline_summary_cgraph_summary *create_ggc (symbol_table *symtab) > + { > +inline_summary_cgraph_summary *summary = new (ggc_cleared_alloc > ()) inline_summary_cgraph_summary(symtab, > true); > +summary->disable_insertion_hook (); > +return summary; > + } > + > + > + virtual void insertion_hook (cgraph_node *, inline_summary *); > + virtual void removal_hook (cgraph_node *node, inline_summary *); > + virtual void duplication_hook (cgraph_node *src, cgraph_node *dst, > inline_summary *src_data, inline_summary *dst_data); > +}; > + > +extern GTY(()) cgraph_summary *inline_summary_summary; All in all it looks better than original code. If we moved insert/ > > /* Information kept about parameter of call site. */ > struct inline_param_summary > @@ -249,10 +265,10 @@ void clone_inlined_nodes (struct cgraph_edge *e, bool, > bool, int *, > extern int ncalls_inlined; > extern int nfunctions_inlined; > > -static inline struct inline_summary * > -inline_summary (struct cgraph_node *node) > +static inline inline_summary * > +get_inline_summary (const struct cgraph_node *node) > { > - return &(*inline_summary_vec)[node->uid]; > + return (*inline_summary_summary)[node->summary_uid]; Hmm, i guess there is no way to avoid the (*...)? Otherwise it would be cleaner to use inline_summary[...] instead of get_inline_summary IMO. Thanks for working on this! Honza
[PATCH 4/4] Data structure is used for inline_summary struct.
gcc/ChangeLog: 2014-11-12 Martin Liska * cgraphunit.c (symbol_table::process_new_functions): inline_summary_vec is replaced with inline_summary_summary. * ipa-cp.c (ipcp_cloning_candidate_p): Usage of get_inline_summary. (devirtualization_time_bonus): Likewise. (estimate_local_effects): Likewise. (ipcp_propagate_stage): Likewise. * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Likewise. (evaluate_properties_for_edge): Likewise. (inline_summary_alloc): Deletion of old hook holders. (reset_inline_summary): inline_summary is added as argument. (inline_summary_cgraph_summary::removal_hook): New function. (inline_summary_cgraph_summary::duplication_hook): Likewise. (dump_inline_edge_summary): Struct keyword removed. (dump_inline_summary): Likewise. (estimate_function_body_sizes): Usage of get_inline_summary. (compute_inline_parameters): Likewise. (estimate_edge_devirt_benefit): Struct keyword removed. (estimate_node_size_and_time): Likewise. (inline_update_callee_summaries): Likewise. (inline_merge_summary): Usage of get_inline_summary. (inline_update_overall_summary): Likewise. (simple_edge_hints): Likewise. (do_estimate_edge_time): Likewise. (estimate_time_after_inlining): Likewise. (estimate_size_after_inlining): Likewise. (do_estimate_growth): Likewise. (growth_likely_positive): Likewise. (inline_generate_summary): inline_summary_summary is registered. (inline_read_section): Struct keyword removed. (inline_read_summary): Likewise. (inline_write_summary): Likewise. (inline_free_summary): Removal of old hook holders. * ipa-inline-transform.c (clone_inlined_nodes): Usage of get_inline_summary. (inline_call): Likewise. * ipa-inline.c (caller_growth_limits): Struct keyword is removed. (can_inline_edge_p): Usage of get_inline_summary. (want_early_inline_function_p): Struct keyword removed. (compute_uninlined_call_time): Usage of get_inline_summary. (compute_inlined_call_time): Likewise. (big_speedup_p): Likewise. (want_inline_small_function_p): Likewise. (edge_badness): Likewise. (update_caller_keys): Likewise. (update_callee_keys): Likewise. (recursive_inlining): Likewise. (inline_small_functions): Likewise. (inline_to_all_callers): Likewise. (dump_overall_stats): Likewise. (early_inline_small_functions): Likewise. * ipa-inline.h (get_inline_summary): New function. * ipa-split.c (execute_split_functions): Usage of get_inline_summary. * ipa.c (walk_polymorphic_call_targets): inline_summary_vec is replaced with inline_summary_summary. * tree-sra.c (ipa_sra_preliminary_function_checks): Usage of get_inline_summary. gcc/lto/ChangeLog: 2014-11-12 Martin Liska * lto-partition.c (add_symbol_to_partition_1): Usage of get_inline_summary. (undo_partition): Likewise. (lto_balanced_map): Likewise. --- gcc/cgraphunit.c | 2 +- gcc/ipa-cp.c | 10 +-- gcc/ipa-inline-analysis.c | 193 - gcc/ipa-inline-transform.c | 6 +- gcc/ipa-inline.c | 61 +++--- gcc/ipa-inline.h | 30 +-- gcc/ipa-split.c| 2 +- gcc/ipa.c | 2 +- gcc/lto/lto-partition.c| 10 +-- gcc/tree-sra.c | 2 +- 10 files changed, 154 insertions(+), 164 deletions(-) diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c index dbbdc44..de032c1 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -338,7 +338,7 @@ symbol_table::process_new_functions (void) if (state == IPA_SSA && !gimple_in_ssa_p (DECL_STRUCT_FUNCTION (fndecl))) g->get_passes ()->execute_early_local_passes (); - else if (inline_summary_vec != NULL) + else if (inline_summary_summary != NULL) compute_inline_parameters (node, true); free_dominance_info (CDI_POST_DOMINATORS); free_dominance_info (CDI_DOMINATORS); diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index da589af..f721e72 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -552,7 +552,7 @@ ipcp_cloning_candidate_p (struct cgraph_node *node) init_caller_stats (&stats); node->call_for_symbol_thunks_and_aliases (gather_caller_stats, &stats, false); - if (inline_summary (node)->self_size < stats.n_calls) + if (get_inline_summary (node)->self_size < stats.n_calls) { if (dump_file) fprintf (dump_file, "Considering %s for cloning; code might shrink.\n", @@ -1718,7 +1718,7 @@ devirtualization_time_bonus (struct cgraph_node *node, for (ie = node->indirect_calls; ie; ie = ie->next_callee) { struct cgrap