diff mbox

Fix for PR ipa/64693

Message ID 54EF2A61.9080104@suse.cz
State New
Headers show

Commit Message

Martin Liška Feb. 26, 2015, 2:14 p.m. UTC
On 02/25/2015 07:46 PM, Jan Hubicka wrote:
>>
>
>> >From dd240028726cb7fdc777acd0b6d14c4f89aed714 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
>>
>> 2015-02-25  Martin Liska  <mliska@suse.cz>
>> 	    Jan Hubicka  <hubicka@ucw.cz>
>>
>> 	* gcc.dg/ipa/ipa-icf-26.c: Update test.
>> 	* gcc.dg/ipa/ipa-icf-33.c: Remove redundant line.
>> 	* gcc.dg/ipa/ipa-icf-34.c: New test.
>>
>> gcc/ChangeLog:
>>
>> 2015-02-25  Martin Liska  <mliska@suse.cz>
>> 	    Jan Hubicka  <hubicka@ucw.cz>
>>
>> 	* cgraph.h (address_matters_p): New function.
>> 	* ipa-icf.c (symbol_compare_collection::symbol_compare_collection): New.
>> 	(sem_item_optimizer::subdivide_classes_by_sensitive_refs): New function.
>> 	(sem_item_optimizer::process_cong_reduction): Include division by
>> 	sensitive references.
>> 	* ipa-icf.h (struct symbol_compare_hashmap_traits): New class.
>> 	* ipa-visibility.c (symtab_node::address_taken_from_non_vtable_p): Removed.
>> 	* symtab.c (address_matters_1):  New function.
>> 	(symtab_node::address_matters_p): Moved from ipa-visibility.c.
>> +  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 && ref->referred->address_matters_p ()
>> +	  && !DECL_VIRTUAL_P (ref->referring->decl))
> !address_matters_p should be implied by !DECL_VIRTUAL_P (ref->referring->decl).
>> +	m_references.safe_push (ref->referred);
>> +
>> +      if (ref->referred->get_availability () <= AVAIL_INTERPOSABLE)
>> +	m_interposables.safe_push (ref->referred);
> Push into m_references if ref->use is IPA_REF_ADDR.  We care about address and not value then.
>> +    }
>> +
>> +  if (is_a <cgraph_node *> (node))
>> +    {
>> +      cgraph_node *cnode = dyn_cast <cgraph_node *> (node);
>> +
>> +      for (cgraph_edge *e = cnode->callees; e; e = e->next_callee)
>> +	if (e->callee->get_availability () <= AVAIL_INTERPOSABLE)
>> +	  m_interposables.safe_push (e->callee);
>> +    }
>> @@ -140,6 +204,15 @@ public:
>>        contains_polymorphic_type_p comparison.  */
>>     static bool get_base_types (tree *t1, tree *t2);
>>
>> +  /* Return true if given DECL is neither virtual nor cdtor.  */
>> +  static bool is_nonvirtual_or_cdtor (tree decl)
>
> You should be able to drop this one.
>> +/* Return ture if address of N is possibly compared.  */
>> +
>> +static bool
>> +address_matters_1 (symtab_node *n, void *)
>> +{
>> +  if (DECL_VIRTUAL_P (n->decl))
>> +    return false;
>> +  if (is_a <cgraph_node *> (n)
>> +      && (DECL_CXX_CONSTRUCTOR_P (n->decl)
>> +	  || DECL_CXX_DESTRUCTOR_P (n->decl)))
>> +    return false;
>> +  if (n->externally_visible
>> +      || n->symtab_node::address_taken_from_non_vtable_p ())
>> +    return true;
>> +  return false;
>> +}
>
> Aha, I meant adding address_matters_p predicate into ipa-ref that will test whether given refernece may lead
> to address being used for comparsion.
>
> Something like
>
> /* Return true if refernece may be used in address compare.  */
> bool
> ipa_ref::address_matters_p ()
> {
>    if (use != IPA_REF_ADDR)
>      return false;
>    /* Addresses taken from virtual tables are never compared.  */
>    if (is_a <varpool_node *> (referring)
>        && DECL_VIRTUAL_P (referring->decl))
>      return false;
>    /* Address of virtual tables and functions is never compared.  */
>    if (DECL_VIRTUAL_P (referred->decl)
>      return false;
>    /* Address of C++ cdtors is never compared.  */
>    if (is_a <cgraph_node *> (referred)
>        && (DECL_CXX_CONSTRUCTOR_P (referred->decl) || DECL_CXX_DESTRUCTOR_P (referred->decl)))
>      return false;
>    return true;
> }
>
> Honza
>

Hi.

There's fixed version of the patch, patch can LTO-boostrap and no regression
has been seen.

Martin
diff mbox

Patch

From d92ae52411e37c472eea04bc27ebd70db92745bc Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Thu, 19 Feb 2015 16:08:09 +0100
Subject: [PATCH 1/4] Fix PR ipa/64693

gcc/ChangeLog:

2015-02-25  Martin Liska  <mliska@suse.cz>
	    Jan Hubicka  <hubicka@ucw.cz>

	PR ipa/64693
	* ipa-icf.c (symbol_compare_collection::symbol_compare_collection): New.
	(sem_item_optimizer::subdivide_classes_by_sensitive_refs): New function.
	(sem_item_optimizer::process_cong_reduction): Include division by
	sensitive references.
	* ipa-icf.h (struct symbol_compare_hashmap_traits): New class.
	* ipa-ref.c (ipa_ref::address_matters_p): New function.
	* ipa-ref.h (ipa_ref::address_matters_p): Likewise.

gcc/testsuite/ChangeLog:

2015-02-25  Martin Liska  <mliska@suse.cz>
	    Jan Hubicka  <hubicka@ucw.cz>

	* g++.dg/ipa/pr64146.C: Update expected results.
	* gcc.dg/ipa/ipa-icf-26.c: Update test.
	* gcc.dg/ipa/ipa-icf-33.c: Remove redundant line.
	* gcc.dg/ipa/ipa-icf-34.c: New test.
---
 gcc/ipa-icf.c                         | 125 ++++++++++++++++++++++++++++++++++
 gcc/ipa-icf.h                         |  71 +++++++++++++++++++
 gcc/ipa-ref.c                         |  20 ++++++
 gcc/ipa-ref.h                         |   3 +
 gcc/testsuite/g++.dg/ipa/pr64146.C    |   3 +-
 gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c |   3 +-
 gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c |   1 -
 gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c |  43 ++++++++++++
 8 files changed, 264 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index e1af8bf..f34407c 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -126,6 +126,44 @@  along with GCC; see the file COPYING3.  If not see
 using namespace ipa_icf_gimple;
 
 namespace ipa_icf {
+
+/* Constructor.  */
+
+symbol_compare_collection::symbol_compare_collection (symtab_node *node)
+{
+  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->address_matters_p ())
+	m_references.safe_push (ref->referred);
+
+      if (ref->referred->get_availability () <= AVAIL_INTERPOSABLE)
+        {
+	  if (ref->use == IPA_REF_ADDR)
+	    m_references.safe_push (ref->referred);
+	  else
+	    m_interposables.safe_push (ref->referred);
+	}
+    }
+
+  if (is_a <cgraph_node *> (node))
+    {
+      cgraph_node *cnode = dyn_cast <cgraph_node *> (node);
+
+      for (cgraph_edge *e = cnode->callees; e; e = e->next_callee)
+	if (e->callee->get_availability () <= AVAIL_INTERPOSABLE)
+	  m_interposables.safe_push (e->callee);
+    }
+}
+
 /* Constructor for key value pair, where _ITEM is key and _INDEX is a target.  */
 
 sem_usage_pair::sem_usage_pair (sem_item *_item, unsigned int _index):
@@ -1967,6 +2005,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.  */
+
+unsigned
+sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
+{
+  unsigned newly_created_classes = 0;
+
+  for (hash_table <congruence_class_group_hash>::iterator it = m_classes.begin ();
+       it != m_classes.end (); ++it)
+    {
+      unsigned int class_count = (*it)->classes.length ();
+      auto_vec<congruence_class *> new_classes;
+
+      for (unsigned i = 0; i < class_count; i++)
+	{
+	  congruence_class *c = (*it)->classes [i];
+
+	  if (c->members.length() > 1)
+	    {
+	      hash_map <symbol_compare_collection *, vec <sem_item *>,
+		symbol_compare_hashmap_traits> split_map;
+
+	      for (unsigned j = 0; j < c->members.length (); j++)
+	        {
+		  sem_item *source_node = c->members[j];
+
+		  symbol_compare_collection *collection = new symbol_compare_collection (source_node->node);
+
+		  vec <sem_item *> *slot = &split_map.get_or_insert (collection);
+		  gcc_checking_assert (slot);
+
+		  slot->safe_push (source_node);
+	        }
+
+	       /* If the map contains more than one key, we have to split the map
+		  appropriately.  */
+	      if (split_map.elements () != 1)
+	        {
+		  bool first_class = true;
+
+		  hash_map <symbol_compare_collection *, vec <sem_item *>,
+		  symbol_compare_hashmap_traits>::iterator it2 = split_map.begin ();
+		  for (; it2 != split_map.end (); ++it2)
+		    {
+		      congruence_class *new_cls;
+		      new_cls = new congruence_class (class_id++);
+
+		      for (unsigned k = 0; k < (*it2).second.length (); k++)
+			add_item_to_class (new_cls, (*it2).second[k]);
+
+		      worklist_push (new_cls);
+		      newly_created_classes++;
+
+		      if (first_class)
+		        {
+			  (*it)->classes[i] = new_cls;
+			  first_class = false;
+			}
+		      else
+		        {
+		          new_classes.safe_push (new_cls);
+			  m_classes_count++;
+		        }
+		    }
+		}
+	    }
+	  }
+
+	for (unsigned i = 0; i < new_classes.length (); i++)
+	  (*it)->classes.safe_push (new_classes[i]);
+    }
+
+  return newly_created_classes;
+}
+
 /* Verify congruence classes if checking is enabled.  */
 
 void
@@ -2256,8 +2372,17 @@  sem_item_optimizer::process_cong_reduction (void)
     fprintf (dump_file, "Congruence class reduction\n");
 
   congruence_class *cls;
+
+  /* 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_sensitive_refs ();
+
+  if (dump_file)
+    fprintf (dump_file, "Address reference subdivision created: %u "
+	     "new classes.\n", new_classes);
 }
 
 /* Debug function prints all informations about congruence classes.  */
diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
index a55699b..9e76239 100644
--- a/gcc/ipa-icf.h
+++ b/gcc/ipa-icf.h
@@ -63,6 +63,70 @@  enum sem_item_type
   VAR
 };
 
+/* Class is container for address references for a symtab_node.  */
+
+class symbol_compare_collection
+{
+public:
+  /* Constructor.  */
+  symbol_compare_collection (symtab_node *node);
+
+  /* Destructor.  */
+  ~symbol_compare_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;
+};
+
+/* Hash traits for symbol_compare_collection map.  */
+
+struct symbol_compare_hashmap_traits: default_hashmap_traits
+{
+  static hashval_t
+  hash (const symbol_compare_collection *v)
+  {
+    inchash::hash hstate;
+    hstate.add_int (v->m_references.length ());
+
+    for (unsigned i = 0; i < v->m_references.length (); i++)
+      hstate.add_ptr (v->m_references[i]->ultimate_alias_target ());
+
+    hstate.add_int (v->m_interposables.length ());
+
+    for (unsigned i = 0; i < v->m_interposables.length (); i++)
+      hstate.add_ptr (v->m_interposables[i]->ultimate_alias_target ());
+
+    return hstate.end ();
+  }
+
+  static bool
+  equal_keys (const symbol_compare_collection *a,
+	      const symbol_compare_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;
+
+    return true;
+  }
+};
+
+
 /* Semantic item usage pair.  */
 class sem_usage_pair
 {
@@ -467,6 +531,13 @@  private:
      classes. If IN_WPA, fast equality function is invoked.  */
   void subdivide_classes_by_equality (bool in_wpa = false);
 
+  /* Subdivide classes by address and interposable 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 subdivide_classes_by_sensitive_refs();
+
   /* Debug function prints all informations about congruence classes.  */
   void dump_cong_classes (void);
 
diff --git a/gcc/ipa-ref.c b/gcc/ipa-ref.c
index 91c2f89..f9af352 100644
--- a/gcc/ipa-ref.c
+++ b/gcc/ipa-ref.c
@@ -124,3 +124,23 @@  ipa_ref::referred_ref_list (void)
 {
   return &referred->ref_list;
 }
+
+/* Return true if refernece may be used in address compare.  */
+bool
+ipa_ref::address_matters_p ()
+{
+  if (use != IPA_REF_ADDR)
+    return false;
+  /* Addresses taken from virtual tables are never compared.  */
+  if (is_a <varpool_node *> (referring)
+      && DECL_VIRTUAL_P (referring->decl))
+    return false;
+  /* Address of virtual tables and functions is never compared.  */
+  if (DECL_VIRTUAL_P (referred->decl))
+    return false;
+  /* Address of C++ cdtors is never compared.  */
+  if (is_a <cgraph_node *> (referred)
+      && (DECL_CXX_CONSTRUCTOR_P (referred->decl) || DECL_CXX_DESTRUCTOR_P (referred->decl)))
+    return false;
+  return true;
+}
diff --git a/gcc/ipa-ref.h b/gcc/ipa-ref.h
index aea7f4c..38df8c9 100644
--- a/gcc/ipa-ref.h
+++ b/gcc/ipa-ref.h
@@ -47,6 +47,9 @@  public:
      function.  */
   bool cannot_lead_to_return ();
 
+  /* Return true if refernece may be used in address compare.  */
+  bool address_matters_p ();
+
   /* Return reference list this reference is in.  */
   struct ipa_ref_list * referring_ref_list (void);
 
diff --git a/gcc/testsuite/g++.dg/ipa/pr64146.C b/gcc/testsuite/g++.dg/ipa/pr64146.C
index bdadfbe..c9a2590 100644
--- a/gcc/testsuite/g++.dg/ipa/pr64146.C
+++ b/gcc/testsuite/g++.dg/ipa/pr64146.C
@@ -34,6 +34,5 @@  int main (int argc, char **argv)
   return 0;
 }
 
-/* { dg-final { scan-ipa-dump-times "Declaration does not bind to currect definition." 2 "icf"  } } */
-/* { dg-final { scan-ipa-dump "Equal symbols: 2" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
 /* { dg-final { cleanup-ipa-dump "icf" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
index 0c5a5a6..f48c040 100644
--- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
@@ -38,7 +38,6 @@  int main()
   return 0;
 }
 
-/* { dg-final { scan-ipa-dump "Semantic equality hit:bar->foo" "icf"  } } */
 /* { dg-final { scan-ipa-dump "Semantic equality hit:remove->destroy" "icf"  } } */
-/* { dg-final { scan-ipa-dump "Equal symbols: 2" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
 /* { dg-final { cleanup-ipa-dump "icf" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
index d7e814d..7b6a8ae 100644
--- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
@@ -23,5 +23,4 @@  int main()
 }
 
 /* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
-/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
 /* { dg-final { cleanup-ipa-dump "icf" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
new file mode 100644
index 0000000..7772e49
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
@@ -0,0 +1,43 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O0 -fipa-icf -fdump-ipa-icf-details"  } */
+
+#include <stdlib.h>
+#include <assert.h>
+
+int callback1(int a)
+{
+  return a * a;
+}
+
+int callback2(int a)
+{
+  return a * a;
+}
+
+static int test(int (*callback) (int))
+{
+  if (callback == callback1)
+    return 1;
+
+  return 0;
+}
+
+int foo()
+{
+  return test(&callback1);
+}
+
+int bar()
+{
+  return test(&callback2);
+}
+
+int main()
+{
+  assert (foo() != bar());
+
+  return 0;
+}
+
+/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2