> Hello. > > There's updated version that reflects how should we handle congruence classes > that have any > address reference. Patch can bootstrap x86_64-linux-pc and no new regression > is introduced? > > Ready for trunk? > Thanks, > Martin > >
> >From d7472e55b345214d55ed49f5f10deafa9a24a4fc Mon Sep 17 00:00:00 2001 > From: mliska <mli...@suse.cz> > Date: Thu, 19 Feb 2015 16:08:09 +0100 > Subject: [PATCH 1/2] Fix PR ipa/64693 > > gcc/ChangeLog: > > 2015-02-20 Martin Liska <mli...@suse.cz> > > PR ipa/64693 > * ipa-icf.c (sem_item_optimizer::add_item_to_class): Identify if > a newly added item has an address reference. > (sem_item_optimizer::subdivide_classes_by_addr_references): > New function. > (sem_item_optimizer::process_cong_reduction): Include subdivision > based on address references. > * ipa-icf.h (struct addr_refs_hashmap_traits): New struct. > * ipa-ref.h (has_addr_ref_p): New function. > > gcc/testsuite/ChangeLog: > > 2015-02-20 Martin Liska <mli...@suse.cz> > > * gcc.dg/ipa/ipa-icf-26.c: Update expected test results. > * gcc.dg/ipa/ipa-icf-33.c: Remove duplicate line. > * gcc.dg/ipa/ipa-icf-34.c: New test. > > diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c > index 494fdcf..859b9d1 100644 > --- a/gcc/ipa-icf.c > +++ b/gcc/ipa-icf.c > @@ -1809,6 +1809,9 @@ sem_item_optimizer::add_item_to_class (congruence_class > *cls, sem_item *item) > item->index_in_class = cls->members.length (); > cls->members.safe_push (item); > item->cls = cls; > + > + if (!cls->has_member_with_addr_ref && item->node->ref_list.has_addr_ref_p > ()) > + cls->has_member_with_addr_ref = true; > } > > /* Congruence classes are built by hash value. */ > @@ -1969,6 +1972,84 @@ sem_item_optimizer::subdivide_classes_by_equality > (bool in_wpa) > verify_classes (); > } > > +/* Subdivide classes by address references that members of the class > + reference. Example can be a pair of functions that have an address > + taken from a function. If these addresses are different the class > + is split. */ OK, I am bit surprised you have a separate loop for this instead of doing it at a place you compare ipa-ref rerences anyway, but I suppose you know the code better than I do ;) > + while(!worklist_empty ()) > + { > + /* Process complete congruence reduction. */ > + while ((cls = worklist_pop ()) != NULL) > + do_congruence_step (cls); > + > + /* Subdivide newly created classes according to references. */ > + unsigned new_classes = subdivide_classes_by_addr_references (); I think this needs to be performed just once, because subdividing does not depend on congurences. > > +/* Class is container for address references for a symtab_node. */ > + > +class addr_refs_collection > +{ > +public: > + addr_refs_collection (symtab_node *node) > + { > + m_references.create (0); > + ipa_ref *ref; > + > + if (is_a <varpool_node *> (node) && DECL_VIRTUAL_P (node->decl)) > + return; > + > + for (unsigned i = 0; i < node->num_references (); i++) > + { > + ref = node->iterate_reference (i, ref); > + if (ref->use == IPA_REF_ADDR) > + m_references.safe_push (ref->referred); You do not need to consider IPA_REF_ADDR of virtual table/ctors/dtors and virtual functions to be address references (because these are never compared for equality.) Test it as The proper conditon on when address matter is if (!DECL_VIRTUAL_P (ref->referred->decl) && (TREE_CODE (ref->referred->decl) != FUNCTION_DECL || (!DECL_CXX_CONSTRUCTOR_P (ref->referred->decl) && !DECL_CXX_DESTRUCTOR_P (ref->referred->decl))) please also update sem_function::merge by adding cdtors in addition to DECL_VIRTUAL_P Why sem_item_optimizer::filter_removed_items checks cdtors? > + } > + } > + > + /* Vector of address references. */ > + vec<symtab_node *> m_references; > +}; > + > +/* Hash traits for addr_refs_collection map. */ > + > +struct addr_refs_hashmap_traits: default_hashmap_traits > +{ > + static hashval_t > + hash (const addr_refs_collection *v) > + { > + inchash::hash hstate; > + hstate.add_int (v->m_references.length ()); > + > + return hstate.end (); This looks like a poor choice of hash function because the count will likely match. equal_address_to basically walks to alias targets A safe approximation is to hash ultimate_alias_target of all entries in your list. > + } > + > + static bool > + equal_keys (const addr_refs_collection *a, > + const addr_refs_collection *b) > + { > + if (a->m_references.length () != b->m_references.length ()) > + return false; > + > + for (unsigned i = 0; i < a->m_references.length (); i++) > + if (a->m_references[i]->equal_address_to (b->m_references[i]) != 1) > + return false; > + > + return true; > + } > +}; OK with these changes. Honza