[PATCH] Fix for PR ipa/64693

[Jan Hubicka]

> 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 <mliska@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  <mliska@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  <mliska@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