Re: [PATCH] Fix for PR ipa/64693

[Jan Hubicka <hubicka at ucw dot cz>]

> 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 <mliska@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  <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.
> 	* ipa-ref.h (has_addr_ref_p): 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..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