From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55197 invoked by alias); 16 Jul 2015 14:06:20 -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 55123 invoked by uid 89); 16 Jul 2015 14:06:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.5 required=5.0 tests=AWL,BAYES_00,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; Thu, 16 Jul 2015 14:06:18 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 57840AB08 for ; Thu, 16 Jul 2015 14:06:15 +0000 (UTC) Message-ID: <55A7BA56.7040709@suse.cz> Date: Thu, 16 Jul 2015 14:08:00 -0000 From: =?UTF-8?B?TWFydGluIExpxaFrYQ==?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: gcc-patches@gcc.gnu.org Subject: Re: [PATCH 4/6] Port ipa-cp to use cgraph_edge summary. References: <20150710141815.GC1819@virgil.suse.cz> In-Reply-To: <20150710141815.GC1819@virgil.suse.cz> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg01405.txt.bz2 On 07/10/2015 04:18 PM, Martin Jambor wrote: > 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 > After a discussion with Martin, we made a decision to preserve current implementation and not to port the IPA CP to he newly introduced summary. Martin