On 07/16/2015 04:05 PM, Martin Liška wrote:
> On 07/10/2015 03:31 PM, Martin Jambor wrote:
>> 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
>>
>
> Hello.
>
> This is v2 of the patch which reflects handy notes by Martin. I decided to
> separate method implementations
> after their declarations. Apart from that, I decided to merge all changes in
> IPA inliner to a single patch.
>
> Thanks,
> Martin
>
>
Hello.
Is the patch ready to be installed to trunk branch?
Thanks,
Martin
