Hi,
thanks for working on this and sorry for a tad late review:
On Thu, Jul 09, 2015 at 11:13:52AM +0200, Martin Liska wrote:
> gcc/ChangeLog:
>
> 2015-07-03 Martin Liska <mli...@suse.cz>
>
> * cgraph.c (symbol_table::create_edge): Introduce summary_uid
> for cgraph_edge.
> * cgraph.h (struct GTY): Likewise.
struct GTY does not look right :-)
> * ipa-inline-analysis.c (estimate_function_body_sizes): Use
> new data structure.
> * ipa-profile.c (ipa_profile): Likewise.
> * ipa-prop.c (ipa_print_node_jump_functions):
Likewise.
> (ipa_propagate_indirect_call_infos): Likewise.
> (ipa_free_edge_args_substructures): Likewise.
> (ipa_free_all_edge_args): Likewise.
> (ipa_edge_args_t::remove): Likewise.
> (ipa_edge_removal_hook): Likewise.
> (ipa_edge_args_t::duplicate): Likewise.
> (ipa_register_cgraph_hooks): Likewise.
> (ipa_unregister_cgraph_hooks): Likewise.
> * ipa-prop.h (ipa_check_create_edge_args): Likewise.
> (ipa_edge_args_info_available_for_edge_p): Likewise.
Definition of ipa_edge_args_t is missing here.
> * symbol-summary.h (gt_ggc_mx): Indent properly.
> (gt_pch_nx): Likewise.
> (edge_summary): New class.
> ---
> gcc/cgraph.c | 2 +
> gcc/cgraph.h | 5 +-
> gcc/ipa-inline-analysis.c | 2 +-
> gcc/ipa-profile.c | 2 +-
> gcc/ipa-prop.c | 71 +++-------------
> gcc/ipa-prop.h | 44 ++++++----
> gcc/symbol-summary.h | 208
> +++++++++++++++++++++++++++++++++++++++++++++-
> 7 files changed, 252 insertions(+), 82 deletions(-)
>
...
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index e6725aa..f0af9b2 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -493,13 +493,36 @@ public:
> extern ipa_node_params_t *ipa_node_params_sum;
> /* Vector of IPA-CP transformation data for each clone. */
> extern GTY(()) vec<ipcp_transformation_summary, va_gc> *ipcp_transformations;
> -/* Vector where the parameter infos are actually stored. */
> -extern GTY(()) vec<ipa_edge_args, va_gc> *ipa_edge_args_vector;
> +
> +/* Function summary for ipa_node_params. */
> +class GTY((user)) ipa_edge_args_t: public edge_summary <ipa_edge_args *>
> +{
> +public:
> + ipa_edge_args_t (symbol_table *symtab):
> + edge_summary <ipa_edge_args*> (symtab, true) { }
> +
> + static ipa_edge_args_t *create_ggc (symbol_table *symtab)
> + {
Please move the body of this function to where the bodies of the rest
of the member functions are.
> + ipa_edge_args_t *summary = new (ggc_cleared_alloc <ipa_edge_args_t> ())
> + ipa_edge_args_t (symtab);
> + return summary;
> + }
> +
> + /* Hook that is called by summary when a node is duplicated. */
> + virtual void duplicate (cgraph_edge *edge,
> + cgraph_edge *edge2,
> + ipa_edge_args *data,
> + ipa_edge_args *data2);
> +
> + virtual void remove (cgraph_edge *edge, ipa_edge_args *data);
> +};
> +
> +extern GTY(()) edge_summary <ipa_edge_args *> *ipa_edge_args_sum;
>
> /* Return the associated parameter/argument info corresponding to the given
> node/edge. */
> #define IPA_NODE_REF(NODE) (ipa_node_params_sum->get (NODE))
> -#define IPA_EDGE_REF(EDGE) (&(*ipa_edge_args_vector)[(EDGE)->uid])
> +#define IPA_EDGE_REF(EDGE) (ipa_edge_args_sum->get (EDGE))
> /* This macro checks validity of index returned by
> ipa_get_param_decl_index function. */
> #define IS_VALID_JUMP_FUNC_INDEX(I) ((I) != -1)
> @@ -532,19 +555,8 @@ ipa_check_create_node_params (void)
> static inline void
> ipa_check_create_edge_args (void)
> {
> - if (vec_safe_length (ipa_edge_args_vector)
> - <= (unsigned) symtab->edges_max_uid)
> - vec_safe_grow_cleared (ipa_edge_args_vector, symtab->edges_max_uid + 1);
> -}
> -
> -/* Returns true if the array of edge infos is large enough to accommodate an
> - info for EDGE. The main purpose of this function is that debug dumping
> - function can check info availability without causing reallocations. */
> -
> -static inline bool
> -ipa_edge_args_info_available_for_edge_p (struct cgraph_edge *edge)
> -{
> - return ((unsigned) edge->uid < vec_safe_length (ipa_edge_args_vector));
> + if (ipa_edge_args_sum == NULL)
> + ipa_edge_args_sum = ipa_edge_args_t::create_ggc (symtab);
> }
>
> static inline ipcp_transformation_summary *
> diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
> index eefbfd9..5799443 100644
> --- a/gcc/symbol-summary.h
> +++ b/gcc/symbol-summary.h
> @@ -108,7 +108,7 @@ public:
> /* Allocates new data that are stored within map. */
> T* allocate_new ()
> {
> - return m_ggc ? new (ggc_alloc <T> ()) T() : new T () ;
> + return m_ggc ? new (ggc_alloc <T> ()) T () : new T () ;
> }
>
> /* Release an item that is stored within map. */
> @@ -234,7 +234,7 @@ private:
>
> template <typename T>
> void
> -gt_ggc_mx(function_summary<T *>* const &summary)
> +gt_ggc_mx (function_summary<T *>* const &summary)
> {
> gcc_checking_assert (summary->m_ggc);
> gt_ggc_mx (&summary->m_map);
> @@ -242,7 +242,7 @@ gt_ggc_mx(function_summary<T *>* const &summary)
>
> template <typename T>
> void
> -gt_pch_nx(function_summary<T *>* const &summary)
> +gt_pch_nx (function_summary<T *>* const &summary)
> {
> gcc_checking_assert (summary->m_ggc);
> gt_pch_nx (&summary->m_map);
> @@ -250,11 +250,211 @@ gt_pch_nx(function_summary<T *>* const &summary)
>
> template <typename T>
> void
> -gt_pch_nx(function_summary<T *>* const& summary, gt_pointer_operator op,
> +gt_pch_nx (function_summary<T *>* const& summary, gt_pointer_operator op,
> void *cookie)
> {
> gcc_checking_assert (summary->m_ggc);
> gt_pch_nx (&summary->m_map, op, cookie);
> }
>
> +/* We want to pass just pointer types as argument for edge_summary
> + template class. */
> +
> +template <class T>
> +class edge_summary
> +{
> +private:
> + edge_summary ();
> +};
> +
Two general remarks about both summary classes. First, they both
certainly need some descriptive comment. I assume this can be added
as a followup.
Second, I'm afraid that having (non-trivial) function definitions
inside the class definition violates our coding conventions
(https://gcc.gnu.org/codingconventions.html#Member_Form). I
understand that, many other classes throughout gcc also have them, but
it seems there is consensus to eradicate it
(https://gcc.gnu.org/ml/gcc/2015-06/msg00241.html) so we at least
should not be adding new cases.
(And I also think that comments need to be separated by a blank line
from the stuff they describe, but that is probably a minor issue, at
least for me.)
> +template <class T>
> +class GTY((user)) edge_summary <T *>
> +{
> +public:
> + /* Default construction takes SYMTAB as an argument. */
> + edge_summary (symbol_table *symtab, bool ggc = false): m_ggc (ggc),
> + m_map (13, ggc), m_symtab (symtab)
> + {
> +#ifdef ENABLE_CHECKING
> + cgraph_node *node;
> +
> + FOR_EACH_FUNCTION (node)
> + {
I'm quite sure you want to verify edges, not nodes, here.
> + gcc_checking_assert (node->summary_uid > 0);
> + }
> +#endif
> +
> + m_symtab_removal_hook =
> + symtab->add_edge_removal_hook
> + (edge_summary::symtab_removal, this);
> + m_symtab_duplication_hook =
> + symtab->add_edge_duplication_hook
> + (edge_summary::symtab_duplication, this);
> + }
> +
> + /* Destructor. */
> + virtual ~edge_summary ()
> + {
> + release ();
> + }
> +
> + /* Destruction method that can be called for GGT purpose. */
> + void release ()
> + {
> + if (m_symtab_removal_hook)
> + m_symtab->remove_edge_removal_hook (m_symtab_removal_hook);
> +
> + if (m_symtab_duplication_hook)
> + m_symtab->remove_edge_duplication_hook (m_symtab_duplication_hook);
> +
> + m_symtab_removal_hook = NULL;
> + m_symtab_duplication_hook = NULL;
> +
> + /* Release all summaries. */
> + typedef typename hash_map <map_hash, T *>::iterator map_iterator;
> + for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
> + release ((*it).second);
> + }
> +
> + /* Traverses all summarys with a function F called with
> + ARG as argument. */
> + template<typename Arg, bool (*f)(const T &, Arg)>
> + void traverse (Arg a) const
> + {
> + m_map.traverse <f> (a);
> + }
> +
> + /* Initializer is called after we allocate a new node. */
We don't allocate a node but an edge.
> + virtual void initialize (cgraph_edge *, T *) {}
> +
> + /* Basic implementation of removal operation. */
> + virtual void remove (cgraph_edge *, T *) {}
> +
> + /* Basic implementation of duplication operation. */
> + virtual void duplicate (cgraph_edge *, cgraph_edge *, T *, T *) {}
Perhaps this should actually be implemented by a simple assignment for
very basic summary types?
> +
> + /* Allocates new data that are stored within map. */
> + T* allocate_new (cgraph_edge *edge)
> + {
> + T *v = m_ggc ? new (ggc_alloc <T> ()) T () : new T () ;
> + initialize (edge, v);
> +
> + return v;
> + }
> +
> + /* Release an item that is stored within map. */
> + void release (T *item)
> + {
> + if (m_ggc)
> + {
> + item->~T ();
> + ggc_free (item);
> + }
> + else
> + delete item;
> + }
> +
> + /* Getter for summary edge node pointer. */
I'd suggest "Getter of edge summary" instead
> + T* get (cgraph_edge *edge)
> + {
> + bool existed;
> + T **v = &m_map.get_or_insert (edge->summary_uid, &existed);
> + if (!existed)
> + *v = allocate_new (edge);
> +
> + return *v;
> + }
> +
> + /* Return number of elements handled by data structure. */
> + size_t elements ()
> + {
> + return m_map.elements ();
> + }
> +
> + /* Symbol removal hook that is registered to symbol table. */
> + static void symtab_removal (cgraph_edge *node, void *data)
Please call the first parameter "edge"
> + {
> + gcc_checking_assert (node->summary_uid);
> + edge_summary *summary = (edge_summary <T *> *) (data);
> +
> + int summary_uid = node->summary_uid;
> + T **v = summary->m_map.get (summary_uid);
> +
> + if (v)
> + {
> + summary->remove (node, *v);
> +
> + if (!summary->m_ggc)
> + delete (*v);
> +
> + summary->m_map.remove (summary_uid);
> + }
> + }
> +
> + /* Symbol duplication hook that is registered to symbol table. */
> + static void symtab_duplication (cgraph_edge *edge, cgraph_edge *edge2,
> + void *data)
> + {
> + edge_summary *summary = (edge_summary <T *> *) (data);
> + T *s = summary->get (edge);
> +
> + gcc_checking_assert (s);
> + gcc_checking_assert (edge2->summary_uid > 0);
> +
> + /* This load is necessary, because we insert a new value! */
What load?
Apart from the above, I'll be very glad to have this class.
Thanks,
Martin
