From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128672 invoked by alias); 10 Jul 2015 14:18:22 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 127224 invoked by uid 89); 10 Jul 2015 14:18:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 X-HELO: mx2.suse.de Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Fri, 10 Jul 2015 14:18:18 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 73CD0AC13 for ; Fri, 10 Jul 2015 14:18:15 +0000 (UTC) Date: Fri, 10 Jul 2015 14:18:00 -0000 From: Martin Jambor To: mliska Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH 4/6] Port ipa-cp to use cgraph_edge summary. Message-ID: <20150710141815.GC1819@virgil.suse.cz> Mail-Followup-To: mliska , gcc-patches@gcc.gnu.org References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg00895.txt.bz2 Hi, I know the patch has been approved by Jeff, but please do not commit it before considering the following: On Thu, Jul 09, 2015 at 11:13:53AM +0200, Martin Liska wrote: > gcc/ChangeLog: > > 2015-07-03 Martin Liska > > * ipa-cp.c (struct edge_clone_summary): New structure. > (class edge_clone_summary_t): Likewise. > (edge_clone_summary_t::initialize): New method. > (edge_clone_summary_t::duplicate): Likewise. > (get_next_cgraph_edge_clone): Remove. > (get_info_about_necessary_edges): Refactor using the new > data structure. > (gather_edges_for_value): Likewise. > (perhaps_add_new_callers): Likewise. > (ipcp_driver): Allocate and deallocate newly added > instance. > --- > gcc/ipa-cp.c | 198 ++++++++++++++++++++++++++++++++++------------------------- > 1 file changed, 113 insertions(+), 85 deletions(-) > > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c > index 16b9cde..8a50b63 100644 > --- a/gcc/ipa-cp.c > +++ b/gcc/ipa-cp.c > @@ -2888,54 +2888,79 @@ ipcp_discover_new_direct_edges (struct cgraph_node *node, > inline_update_overall_summary (node); > } > > -/* Vector of pointers which for linked lists of clones of an original crgaph > - edge. */ > +/* Edge clone summary. */ > > -static vec next_edge_clone; > -static vec prev_edge_clone; > - > -static inline void > -grow_edge_clone_vectors (void) > +struct edge_clone_summary I's got constructors and destructors so it should be a class, reaally. > { > - if (next_edge_clone.length () > - <= (unsigned) symtab->edges_max_uid) > - next_edge_clone.safe_grow_cleared (symtab->edges_max_uid + 1); > - if (prev_edge_clone.length () > - <= (unsigned) symtab->edges_max_uid) > - prev_edge_clone.safe_grow_cleared (symtab->edges_max_uid + 1); > -} > + /* Default constructor. */ > + edge_clone_summary (): edge_set (NULL), edge (NULL) {} > > -/* Edge duplication hook to grow the appropriate linked list in > - next_edge_clone. */ > + /* Default destructor. */ > + ~edge_clone_summary () > + { > + gcc_assert (edge_set != NULL); > > -static void > -ipcp_edge_duplication_hook (struct cgraph_edge *src, struct cgraph_edge *dst, > - void *) > + if (edge != NULL) > + { > + gcc_checking_assert (edge_set->contains (edge)); > + edge_set->remove (edge); > + } > + > + /* Release memory for an empty set. */ > + if (edge_set->elements () == 0) > + delete edge_set; > + } > + > + hash_set *edge_set; > + cgraph_edge *edge; If the hash set is supposed to replace the linked list of edge clones, then a removal mechanism seems to be missing. The whole point of prev_edge_clone vector was to allow removal of edges from the linked list, because as speculative edges are thrown away, clones can be too and then we must remove the pointer from the list, or hash set. Have you tried -O3 LTOing Firefox with these changes? But I must say that I'm not convinced that converting the linked list into a hash_set is a good idea at all. Apart from the self-removal operation, the lists are always traversed linearly and in full, so except for using a C++-style iterator, I really do not see any point. Moreover, you seem to create a hash table for each and every edge, even when it has no clones, just to be able to enter the edge itself into it, and so not skip it when you iterate over all clones. That really seems like unjustifiable overhead. And the deletion in duplication hook is also very unappealing. So the bottom line is that while I like turning the two vectors into a summary, I do not like the hash set at all. If absolutely think it is a good idea, please make that change in a separate patch so that we can better argue about its merits. On the other hand, since the summaries are hash-based themselves, it would be great if they had a predicate to find out whether there is any summary for a given edge at all and have get_next_cgraph_edge_clone return false if there was none. That would actually save memory. Thanks, Martin