Re: [PATCH 4/4] Data structure is used for inline_summary struct.

2014-12-18 Thread Jan Hubicka
> 
> 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.

2014-12-08 Thread Martin Liška

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.

2014-11-19 Thread Martin Liška

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.

2014-11-18 Thread Trevor Saunders
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.

2014-11-18 Thread Martin Jambor
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.

2014-11-18 Thread Jan Hubicka
> 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.

2014-11-18 Thread Martin Jambor
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.

2014-11-18 Thread Jan Hubicka
> 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.

2014-11-18 Thread Martin Jambor
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.

2014-11-18 Thread Martin Liška

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.

2014-11-14 Thread Jan Hubicka
> > 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.

2014-11-14 Thread Martin Jambor
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.

2014-11-14 Thread Jan Hubicka
> >
> >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.

2014-11-14 Thread Martin Liška

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.

2014-11-14 Thread Martin Liška

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.

2014-11-13 Thread Jan Hubicka
> +  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.

2014-11-13 Thread mliska
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