> Hello Honza. > > I've updated the patch so that your notes are resolved. Moreover, I've added > comparison > for interposable symbols that are either target of reference or are called by > a function. > Please read the patch to verify the comparison is as you expected. > > I'm going to run testsuite. > > Thanks, > Martin
> >From 8dae064e67e30537486e0d502fc5df39d37cee3e Mon Sep 17 00:00:00 2001 > From: mliska <mli...@suse.cz> > Date: Thu, 19 Feb 2015 16:08:09 +0100 > Subject: [PATCH 1/3] 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. > (sem_item::is_nonvirtual_or_cdtor): 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..fbb641d 100644 > --- a/gcc/ipa-icf.c > +++ b/gcc/ipa-icf.c > @@ -126,6 +126,40 @@ along with GCC; see the file COPYING3. If not see > using namespace ipa_icf_gimple; > > namespace ipa_icf { > + > +/* Constructor. */ > + > +addr_refs_collection::addr_refs_collection (symtab_node *node) I gues because you now track two thinks, address references and interposable symbols, perhaps the function name can reflect it. Perhaps symbol_compare_collection sounds more precise, but I leave decision on you. > +{ > + m_references.create (0); > + m_interposables.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 > + && sem_item::is_nonvirtual_or_cdtor (ref->referred->decl)) > + m_references.safe_push (ref->referred); Since I introduced the address_matters predicate, just make is_nonvirtual_or_cdtor a address_matters_p predicate of ipa_ref itself. Test that reference is ADDR, referring is not virtual table and referred is is non-virtual noncdotr. It is better to have this centralized in symbol table predicates because later we may want to get smarter. > @@ -638,11 +672,11 @@ sem_function::merge (sem_item *alias_item) > > /* See if original and/or alias address can be compared for equality. */ > original_address_matters > - = (!DECL_VIRTUAL_P (original->decl) > + = (sem_item::is_nonvirtual_or_cdtor (original->decl) > && (original->externally_visible > || original->address_taken_from_non_vtable_p ())); > alias_address_matters > - = (!DECL_VIRTUAL_P (alias->decl) > + = (sem_item::is_nonvirtual_or_cdtor (alias->decl) > && (alias->externally_visible > || alias->address_taken_from_non_vtable_p ())); > Lets levae this for incremental patch for the ::nerge revamp. > @@ -1969,6 +2003,82 @@ 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. */ > + > +unsigned > +sem_item_optimizer::subdivide_classes_by_addr_references () Simialrly this needs update of name. > @@ -2258,8 +2368,20 @@ sem_item_optimizer::process_cong_reduction (void) > fprintf (dump_file, "Congruence class reduction\n"); > > congruence_class *cls; > - while ((cls = worklist_pop ()) != NULL) > - do_congruence_step (cls); > + > + 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 (); Still do not see why this needs to be iterated within the loop and not just executed once ;) > +class addr_refs_collection > +{ > +public: > + /* Constructor. */ > + addr_refs_collection (symtab_node *node); > + > + /* Destructor. */ > + ~addr_refs_collection () > + { > + m_references.release (); > + m_interposables.release (); > + } > + > + /* Vector of address references. */ > + vec<symtab_node *> m_references; > + > + /* Vector of interposable references. */ > + vec<symtab_node *> m_interposables; > +}; > + 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; > + > + for (unsigned i = 0; i < a->m_interposables.length (); i++) > + if (!a->m_interposables[i]->semantically_equivalent_p > + (b->m_interposables[i])) > + return false; Aha, only here I noticed you make difference between references and interposables. ADDR_EXPR of interposable symbol should go to references, too. I think it does not make difference whether you use equal_address_to or semantically_equivalent_p in your setup with current logic in the preidcates, but lets keep it this way; it is more robust. OK with these changes. Honza