[PATCH] Fix for PR ipa/64693

[PATCH] Fix for PR ipa/64693

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

Hello.

There's patch for aforementioned issue where ICF process a merge operation
for a pair of functions that pass a pointer to the second pair of functions
(these functions are also identical). Unfortunately, these addresses are compared
for equality.

Patch can lto-boostrap on x86_64 and pch.exp tests work correctly.

Ready for trunk?
Thanks,
Martin


[-- Attachment #2: 0001-Fix-PR-ipa-64693.patch --]
[-- Type: text/x-patch, Size: 7366 bytes --]

From 7134778d86d85aafc05fc9af9c8a2973516da29c Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Fri, 23 Jan 2015 14:58:36 +0100
Subject: [PATCH] Fix PR ipa/64693.

gcc/ChangeLog:

2015-02-12  Martin Liska  <mliska@suse.cz>

	PR ipa/64693
	* ipa-icf.c (sem_item_optimizer::execute): Call identify_address_sensitive_classes.
	(sem_item_optimizer::identify_address_sensitive_classes): New function.
	(sem_item_optimizer::merge_classes): Handle cases where we merge a class
	with a sensitive reference.
	(congruence_class::dump): Add support for a new sensitive_reference flag.
	* ipa-icf.h: Add new function.

gcc/testsuite/ChangeLog:

2015-01-23  Martin Liska  <mliska@suse.cz>

	* gcc.dg/ipa/ipa-icf-26.c: Fix expected results.
	* gcc.dg/ipa/ipa-icf-33.c: Remove reduced line.
	* gcc.dg/ipa/ipa-icf-34.c: New test.
---
 gcc/ipa-icf.c                         | 71 ++++++++++++++++++++++++++++++++++-
 gcc/ipa-icf.h                         | 10 ++++-
 gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c |  2 +-
 gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c |  1 -
 gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c | 44 ++++++++++++++++++++++
 5 files changed, 123 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 afb5be5..12dd794 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1729,6 +1729,7 @@ sem_item_optimizer::execute (void)
   unsigned int prev_class_count = m_classes_count;
 
   process_cong_reduction ();
+  identify_address_sensitive_classes ();
   dump_cong_classes ();
   verify_classes ();
   merge_classes (prev_class_count);
@@ -2240,6 +2241,61 @@ sem_item_optimizer::process_cong_reduction (void)
     do_congruence_step (cls);
 }
 
+
+/* Identify congruence classes which have an address taken. These
+   classes can be potentially been merged just as thunks.  */
+
+void
+sem_item_optimizer::identify_address_sensitive_classes (void)
+{
+  for (hash_table<congruence_class_group_hash>::iterator it = m_classes.begin ();
+       it != m_classes.end (); ++it)
+    for (unsigned i = 0; i < (*it)->classes.length (); i++)
+      {
+	congruence_class *cls = (*it)->classes[i];
+
+	if (cls->members.length () == 1)
+	  continue;
+
+	auto_vec <sem_item *> references;
+	sem_item *source_node = cls->members[0];
+	ipa_ref *r;
+
+	for (unsigned j = 0; j < source_node->node->num_references (); j++)
+	  {
+	    symtab_node *ref = source_node->node->iterate_reference (j, r)->referred;
+	    sem_item **symbol = m_symtab_node_map.get (ref);
+	    if (symbol)
+	      references.safe_push (*symbol);
+	  }
+
+	for (unsigned j = 1; j < cls->members.length (); j++)
+	  {
+	    source_node = cls->members[j];
+
+	    unsigned l = 0;
+	    for (unsigned k = 0; k < source_node->node->num_references (); k++)
+	      {
+		symtab_node *ref = source_node->node->iterate_reference (k, r)->referred;
+		sem_item **symbol = m_symtab_node_map.get (ref);
+		if (symbol)
+		  {
+		    if (l >= references.length () || references[l] != *symbol)
+		      {
+			cls->sensitive_reference = true;
+			break;
+		      }
+
+		    l++;
+		  }
+	      }
+
+	    if (cls->sensitive_reference)
+	      break;
+	  }
+      }
+}
+
 /* Debug function prints all informations about congruence classes.  */
 
 void
@@ -2374,6 +2430,15 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
 		continue;
 	      }
 
+	    if (c->sensitive_reference)
+	      {
+		if (dump_file)
+		  fprintf (dump_file, "A function from the congruence class "
+			   "uses a sensitive reference.\n");
+
+		continue;
+	      }
+
 	    if (dump_file && (dump_flags & TDF_DETAILS))
 	      {
 		source->dump_to_file (dump_file);
@@ -2390,8 +2455,10 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
 void
 congruence_class::dump (FILE *file, unsigned int indent) const
 {
-  FPRINTF_SPACES (file, indent, "class with id: %u, hash: %u, items: %u\n",
-		  id, members[0]->get_hash (), members.length ());
+  FPRINTF_SPACES (file, indent,
+		  "class with id: %u, hash: %u, items: %u %s\n",
+		  id, members[0]->get_hash (), members.length (),
+		  sensitive_reference ? "sensitive_reference" : "");
 
   FPUTS_SPACES (file, indent + 2, "");
   for (unsigned i = 0; i < members.length (); i++)
diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
index adbedd6..db0bc54 100644
--- a/gcc/ipa-icf.h
+++ b/gcc/ipa-icf.h
@@ -29,7 +29,8 @@ class congruence_class
 {
 public:
   /* Congruence class constructor for a new class with _ID.  */
-  congruence_class (unsigned int _id): in_worklist (false), id(_id)
+  congruence_class (unsigned int _id): in_worklist (false), id(_id),
+    sensitive_reference (false)
   {
   }
 
@@ -54,6 +55,9 @@ public:
 
   /* Global unique class identifier.  */
   unsigned int id;
+
+  /* A function from the class contains a reference to another class.  */
+  bool sensitive_reference;
 };
 
 /* Semantic item type enum.  */
@@ -476,6 +480,10 @@ private:
   /* Iterative congruence reduction function.  */
   void process_cong_reduction (void);
 
+  /* Identify congruence classes which have an address taken. These
+     classes can be potentially been merged just as thunks.  */
+  void identify_address_sensitive_classes (void);
+
   /* After reduction is done, we can declare all items in a group
      to be equal. PREV_CLASS_COUNT is start number of classes
      before reduction.  */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
index 0c5a5a6..f25a251 100644
--- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
@@ -38,7 +38,7 @@ 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 "A function from the congruence class uses a sensitive reference." "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..66bcac5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
@@ -0,0 +1,44 @@
+/* { 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: 2" "icf"  } } */
+/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2

Re: [PATCH] Fix for PR ipa/64693

[Jan Hubicka]

> From: mliska <mliska@suse.cz>V
> Date: Fri, 23 Jan 2015 14:58:36 +0100
> Subject: [PATCH] Fix PR ipa/64693.
> 
> gcc/ChangeLog:
> 
> 2015-02-12  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/64693
> 	* ipa-icf.c (sem_item_optimizer::execute): Call identify_address_sensitive_classes.
> 	(sem_item_optimizer::identify_address_sensitive_classes): New function.
> 	(sem_item_optimizer::merge_classes): Handle cases where we merge a class
> 	with a sensitive reference.
> 	(congruence_class::dump): Add support for a new sensitive_reference flag.
> 	* ipa-icf.h: Add new function.
> 
> +
> +/* Identify congruence classes which have an address taken. These
> +   classes can be potentially been merged just as thunks.  */

Hmm, I would expect someting like this

 1) break out code in sem_function::merge which decides if function A and B
    can be identified or if thunk is needed
 2) when comparing references to verify congruence, actually check if merging
    would result in thunk and subdivide.

In the following:

void f1()
{
}
void f2()
{
}
void *a=&f1;
void *b=&f1;
void *c=&f2;
void *d=&f2;

The code bellow seems to disable all merging, while we ought to be able to mere A,B
and C,D into two congruences.
Similarly we ought to be able to merge in a case F2 is a virtual function/ctor/dtor
where address can not be compared for equality.
> +
> +void
> +sem_item_optimizer::identify_address_sensitive_classes (void)
> +{
> +  for (hash_table<congruence_class_group_hash>::iterator it = m_classes.begin ();
> +       it != m_classes.end (); ++it)
> +    for (unsigned i = 0; i < (*it)->classes.length (); i++)
> +      {
> +	congruence_class *cls = (*it)->classes[i];
> +
> +	if (cls->members.length () == 1)
> +	  continue;
> +
> +	auto_vec <sem_item *> references;
> +	sem_item *source_node = cls->members[0];
> +	ipa_ref *r;
> +
> +	for (unsigned j = 0; j < source_node->node->num_references (); j++)
> +	  {
> +	    symtab_node *ref = source_node->node->iterate_reference (j, r)->referred;
> +	    sem_item **symbol = m_symtab_node_map.get (ref);
> +	    if (symbol)
> +	      references.safe_push (*symbol);
> +	  }
> +
> +	for (unsigned j = 1; j < cls->members.length (); j++)
> +	  {
> +	    source_node = cls->members[j];
> +
> +	    unsigned l = 0;
> +	    for (unsigned k = 0; k < source_node->node->num_references (); k++)
> +	      {
> +		symtab_node *ref = source_node->node->iterate_reference (k, r)->referred;
> +		sem_item **symbol = m_symtab_node_map.get (ref);
> +		if (symbol)
> +		  {
> +		    if (l >= references.length () || references[l] != *symbol)
> +		      {
> +			cls->sensitive_reference = true;
> +			break;
> +		      }
> +
> +		    l++;
> +		  }
> +	      }
> +
> +	    if (cls->sensitive_reference)
> +	      break;
> +	  }
> +      }
> +}
> +
>  /* Debug function prints all informations about congruence classes.  */
>  
>  void
> @@ -2374,6 +2430,15 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>  		continue;
>  	      }
>  
> +	    if (c->sensitive_reference)
> +	      {
> +		if (dump_file)
> +		  fprintf (dump_file, "A function from the congruence class "
> +			   "uses a sensitive reference.\n");
> +
> +		continue;
> +	      }
> +
>  	    if (dump_file && (dump_flags & TDF_DETAILS))
>  	      {
>  		source->dump_to_file (dump_file);
> @@ -2390,8 +2455,10 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>  void
>  congruence_class::dump (FILE *file, unsigned int indent) const
>  {
> -  FPRINTF_SPACES (file, indent, "class with id: %u, hash: %u, items: %u\n",
> -		  id, members[0]->get_hash (), members.length ());
> +  FPRINTF_SPACES (file, indent,
> +		  "class with id: %u, hash: %u, items: %u %s\n",
> +		  id, members[0]->get_hash (), members.length (),
> +		  sensitive_reference ? "sensitive_reference" : "");
>  
>    FPUTS_SPACES (file, indent + 2, "");
>    for (unsigned i = 0; i < members.length (); i++)
> diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
> index adbedd6..db0bc54 100644
> --- a/gcc/ipa-icf.h
> +++ b/gcc/ipa-icf.h
> @@ -29,7 +29,8 @@ class congruence_class
>  {
>  public:
>    /* Congruence class constructor for a new class with _ID.  */
> -  congruence_class (unsigned int _id): in_worklist (false), id(_id)
> +  congruence_class (unsigned int _id): in_worklist (false), id(_id),
> +    sensitive_reference (false)
>    {
>    }
>  
> @@ -54,6 +55,9 @@ public:
>  
>    /* Global unique class identifier.  */
>    unsigned int id;
> +
> +  /* A function from the class contains a reference to another class.  */
> +  bool sensitive_reference;
>  };
>  
>  /* Semantic item type enum.  */
> @@ -476,6 +480,10 @@ private:
>    /* Iterative congruence reduction function.  */
>    void process_cong_reduction (void);
>  
> +  /* Identify congruence classes which have an address taken. These
> +     classes can be potentially been merged just as thunks.  */
> +  void identify_address_sensitive_classes (void);
> +
>    /* After reduction is done, we can declare all items in a group
>       to be equal. PREV_CLASS_COUNT is start number of classes
>       before reduction.  */
> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
> index 0c5a5a6..f25a251 100644
> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
> @@ -38,7 +38,7 @@ 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 "A function from the congruence class uses a sensitive reference." "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..66bcac5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
> @@ -0,0 +1,44 @@
> +/* { 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: 2" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf"  } } */
> +/* { dg-final { cleanup-ipa-dump "icf" } } */
> -- 
> 2.1.2
> 

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

On 02/12/2015 05:57 PM, Jan Hubicka wrote:
>> From: mliska <mliska@suse.cz>V
>> Date: Fri, 23 Jan 2015 14:58:36 +0100
>> Subject: [PATCH] Fix PR ipa/64693.
>>
>> gcc/ChangeLog:
>>
>> 2015-02-12  Martin Liska  <mliska@suse.cz>
>>
>> 	PR ipa/64693
>> 	* ipa-icf.c (sem_item_optimizer::execute): Call identify_address_sensitive_classes.
>> 	(sem_item_optimizer::identify_address_sensitive_classes): New function.
>> 	(sem_item_optimizer::merge_classes): Handle cases where we merge a class
>> 	with a sensitive reference.
>> 	(congruence_class::dump): Add support for a new sensitive_reference flag.
>> 	* ipa-icf.h: Add new function.
>>
>> +
>> +/* Identify congruence classes which have an address taken. These
>> +   classes can be potentially been merged just as thunks.  */
>
> Hmm, I would expect someting like this
>
>   1) break out code in sem_function::merge which decides if function A and B
>      can be identified or if thunk is needed
>   2) when comparing references to verify congruence, actually check if merging
>      would result in thunk and subdivide.
>
> In the following:
>
> void f1()
> {
> }
> void f2()
> {
> }
> void *a=&f1;
> void *b=&f1;
> void *c=&f2;
> void *d=&f2;
>
> The code bellow seems to disable all merging, while we ought to be able to mere A,B
> and C,D into two congruences.
> Similarly we ought to be able to merge in a case F2 is a virtual function/ctor/dtor
> where address can not be compared for equality.

Hello.

The code below works fine with this example:
Semantic equality hit:d->c
Semantic equality hit:b->a
Semantic equality hit:f2->f1

It just provides guard for following very special situation (cc1 LTO contains just 2 classes with this example),
which is in gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c:

Where we have functions in these classes:
class 1:
   foo,bar

class 2:
   callback1, callback2

Where foo references callback1 and bar points to callback2. As callback{1,2} are in the same congruence class
we can process merge operation of foo and bar. Unfortunately, address of callback{1,2} is taken and in this case
we cannot merge foo and bar.

As you said, to make things really correct, one should split class 1 according to these references.
Question is if we want to do it for GCC 5.0 or prepare if for next release?

I'm going to measure number of occurrences in Firefox and can make a decision.
I like the idea of separating function in a class to candidates that need thunk creation and the rest. That will
work seamlessly with your idea to include all functions that are just 'wrappers' for a function.

Thanks for ideas,
Martin

>> +
>> +void
>> +sem_item_optimizer::identify_address_sensitive_classes (void)
>> +{
>> +  for (hash_table<congruence_class_group_hash>::iterator it = m_classes.begin ();
>> +       it != m_classes.end (); ++it)
>> +    for (unsigned i = 0; i < (*it)->classes.length (); i++)
>> +      {
>> +	congruence_class *cls = (*it)->classes[i];
>> +
>> +	if (cls->members.length () == 1)
>> +	  continue;
>> +
>> +	auto_vec <sem_item *> references;
>> +	sem_item *source_node = cls->members[0];
>> +	ipa_ref *r;
>> +
>> +	for (unsigned j = 0; j < source_node->node->num_references (); j++)
>> +	  {
>> +	    symtab_node *ref = source_node->node->iterate_reference (j, r)->referred;
>> +	    sem_item **symbol = m_symtab_node_map.get (ref);
>> +	    if (symbol)
>> +	      references.safe_push (*symbol);
>> +	  }
>> +
>> +	for (unsigned j = 1; j < cls->members.length (); j++)
>> +	  {
>> +	    source_node = cls->members[j];
>> +
>> +	    unsigned l = 0;
>> +	    for (unsigned k = 0; k < source_node->node->num_references (); k++)
>> +	      {
>> +		symtab_node *ref = source_node->node->iterate_reference (k, r)->referred;
>> +		sem_item **symbol = m_symtab_node_map.get (ref);
>> +		if (symbol)
>> +		  {
>> +		    if (l >= references.length () || references[l] != *symbol)
>> +		      {
>> +			cls->sensitive_reference = true;
>> +			break;
>> +		      }
>> +
>> +		    l++;
>> +		  }
>> +	      }
>> +
>> +	    if (cls->sensitive_reference)
>> +	      break;
>> +	  }
>> +      }
>> +}
>> +
>>   /* Debug function prints all informations about congruence classes.  */
>>
>>   void
>> @@ -2374,6 +2430,15 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>   		continue;
>>   	      }
>>
>> +	    if (c->sensitive_reference)
>> +	      {
>> +		if (dump_file)
>> +		  fprintf (dump_file, "A function from the congruence class "
>> +			   "uses a sensitive reference.\n");
>> +
>> +		continue;
>> +	      }
>> +
>>   	    if (dump_file && (dump_flags & TDF_DETAILS))
>>   	      {
>>   		source->dump_to_file (dump_file);
>> @@ -2390,8 +2455,10 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>   void
>>   congruence_class::dump (FILE *file, unsigned int indent) const
>>   {
>> -  FPRINTF_SPACES (file, indent, "class with id: %u, hash: %u, items: %u\n",
>> -		  id, members[0]->get_hash (), members.length ());
>> +  FPRINTF_SPACES (file, indent,
>> +		  "class with id: %u, hash: %u, items: %u %s\n",
>> +		  id, members[0]->get_hash (), members.length (),
>> +		  sensitive_reference ? "sensitive_reference" : "");
>>
>>     FPUTS_SPACES (file, indent + 2, "");
>>     for (unsigned i = 0; i < members.length (); i++)
>> diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
>> index adbedd6..db0bc54 100644
>> --- a/gcc/ipa-icf.h
>> +++ b/gcc/ipa-icf.h
>> @@ -29,7 +29,8 @@ class congruence_class
>>   {
>>   public:
>>     /* Congruence class constructor for a new class with _ID.  */
>> -  congruence_class (unsigned int _id): in_worklist (false), id(_id)
>> +  congruence_class (unsigned int _id): in_worklist (false), id(_id),
>> +    sensitive_reference (false)
>>     {
>>     }
>>
>> @@ -54,6 +55,9 @@ public:
>>
>>     /* Global unique class identifier.  */
>>     unsigned int id;
>> +
>> +  /* A function from the class contains a reference to another class.  */
>> +  bool sensitive_reference;
>>   };
>>
>>   /* Semantic item type enum.  */
>> @@ -476,6 +480,10 @@ private:
>>     /* Iterative congruence reduction function.  */
>>     void process_cong_reduction (void);
>>
>> +  /* Identify congruence classes which have an address taken. These
>> +     classes can be potentially been merged just as thunks.  */
>> +  void identify_address_sensitive_classes (void);
>> +
>>     /* After reduction is done, we can declare all items in a group
>>        to be equal. PREV_CLASS_COUNT is start number of classes
>>        before reduction.  */
>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> index 0c5a5a6..f25a251 100644
>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> @@ -38,7 +38,7 @@ 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 "A function from the congruence class uses a sensitive reference." "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..66bcac5
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>> @@ -0,0 +1,44 @@
>> +/* { 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: 2" "icf"  } } */
>> +/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf"  } } */
>> +/* { dg-final { cleanup-ipa-dump "icf" } } */
>> --
>> 2.1.2
>>
>

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

On 02/13/2015 02:37 PM, Martin Liška wrote:
> On 02/12/2015 05:57 PM, Jan Hubicka wrote:
>>> From: mliska <mliska@suse.cz>V
>>> Date: Fri, 23 Jan 2015 14:58:36 +0100
>>> Subject: [PATCH] Fix PR ipa/64693.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2015-02-12  Martin Liska  <mliska@suse.cz>
>>>
>>>     PR ipa/64693
>>>     * ipa-icf.c (sem_item_optimizer::execute): Call identify_address_sensitive_classes.
>>>     (sem_item_optimizer::identify_address_sensitive_classes): New function.
>>>     (sem_item_optimizer::merge_classes): Handle cases where we merge a class
>>>     with a sensitive reference.
>>>     (congruence_class::dump): Add support for a new sensitive_reference flag.
>>>     * ipa-icf.h: Add new function.
>>>
>>> +
>>> +/* Identify congruence classes which have an address taken. These
>>> +   classes can be potentially been merged just as thunks.  */
>>
>> Hmm, I would expect someting like this
>>
>>   1) break out code in sem_function::merge which decides if function A and B
>>      can be identified or if thunk is needed
>>   2) when comparing references to verify congruence, actually check if merging
>>      would result in thunk and subdivide.
>>
>> In the following:
>>
>> void f1()
>> {
>> }
>> void f2()
>> {
>> }
>> void *a=&f1;
>> void *b=&f1;
>> void *c=&f2;
>> void *d=&f2;
>>
>> The code bellow seems to disable all merging, while we ought to be able to mere A,B
>> and C,D into two congruences.
>> Similarly we ought to be able to merge in a case F2 is a virtual function/ctor/dtor
>> where address can not be compared for equality.
>
> Hello.
>
> The code below works fine with this example:
> Semantic equality hit:d->c
> Semantic equality hit:b->a
> Semantic equality hit:f2->f1
>
> It just provides guard for following very special situation (cc1 LTO contains just 2 classes with this example),
> which is in gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c:
>
> Where we have functions in these classes:
> class 1:
>    foo,bar
>
> class 2:
>    callback1, callback2
>
> Where foo references callback1 and bar points to callback2. As callback{1,2} are in the same congruence class
> we can process merge operation of foo and bar. Unfortunately, address of callback{1,2} is taken and in this case
> we cannot merge foo and bar.
>
> As you said, to make things really correct, one should split class 1 according to these references.
> Question is if we want to do it for GCC 5.0 or prepare if for next release?
>
> I'm going to measure number of occurrences in Firefox and can make a decision.
> I like the idea of separating function in a class to candidates that need thunk creation and the rest. That will
> work seamlessly with your idea to include all functions that are just 'wrappers' for a function.
>
> Thanks for ideas,
> Martin

I forgot to mention that I have to update your test to:
void f1()
{
}
void f2()
{
}

static void* const a=&f1;
static void* const b=&f1;
static void* const c=&f2;
static void* const d=&f2;

Const static variables are necessary to enable merge operation.

Martin

>
>>> +
>>> +void
>>> +sem_item_optimizer::identify_address_sensitive_classes (void)
>>> +{
>>> +  for (hash_table<congruence_class_group_hash>::iterator it = m_classes.begin ();
>>> +       it != m_classes.end (); ++it)
>>> +    for (unsigned i = 0; i < (*it)->classes.length (); i++)
>>> +      {
>>> +    congruence_class *cls = (*it)->classes[i];
>>> +
>>> +    if (cls->members.length () == 1)
>>> +      continue;
>>> +
>>> +    auto_vec <sem_item *> references;
>>> +    sem_item *source_node = cls->members[0];
>>> +    ipa_ref *r;
>>> +
>>> +    for (unsigned j = 0; j < source_node->node->num_references (); j++)
>>> +      {
>>> +        symtab_node *ref = source_node->node->iterate_reference (j, r)->referred;
>>> +        sem_item **symbol = m_symtab_node_map.get (ref);
>>> +        if (symbol)
>>> +          references.safe_push (*symbol);
>>> +      }
>>> +
>>> +    for (unsigned j = 1; j < cls->members.length (); j++)
>>> +      {
>>> +        source_node = cls->members[j];
>>> +
>>> +        unsigned l = 0;
>>> +        for (unsigned k = 0; k < source_node->node->num_references (); k++)
>>> +          {
>>> +        symtab_node *ref = source_node->node->iterate_reference (k, r)->referred;
>>> +        sem_item **symbol = m_symtab_node_map.get (ref);
>>> +        if (symbol)
>>> +          {
>>> +            if (l >= references.length () || references[l] != *symbol)
>>> +              {
>>> +            cls->sensitive_reference = true;
>>> +            break;
>>> +              }
>>> +
>>> +            l++;
>>> +          }
>>> +          }
>>> +
>>> +        if (cls->sensitive_reference)
>>> +          break;
>>> +      }
>>> +      }
>>> +}
>>> +
>>>   /* Debug function prints all informations about congruence classes.  */
>>>
>>>   void
>>> @@ -2374,6 +2430,15 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>>           continue;
>>>             }
>>>
>>> +        if (c->sensitive_reference)
>>> +          {
>>> +        if (dump_file)
>>> +          fprintf (dump_file, "A function from the congruence class "
>>> +               "uses a sensitive reference.\n");
>>> +
>>> +        continue;
>>> +          }
>>> +
>>>           if (dump_file && (dump_flags & TDF_DETAILS))
>>>             {
>>>           source->dump_to_file (dump_file);
>>> @@ -2390,8 +2455,10 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>>   void
>>>   congruence_class::dump (FILE *file, unsigned int indent) const
>>>   {
>>> -  FPRINTF_SPACES (file, indent, "class with id: %u, hash: %u, items: %u\n",
>>> -          id, members[0]->get_hash (), members.length ());
>>> +  FPRINTF_SPACES (file, indent,
>>> +          "class with id: %u, hash: %u, items: %u %s\n",
>>> +          id, members[0]->get_hash (), members.length (),
>>> +          sensitive_reference ? "sensitive_reference" : "");
>>>
>>>     FPUTS_SPACES (file, indent + 2, "");
>>>     for (unsigned i = 0; i < members.length (); i++)
>>> diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
>>> index adbedd6..db0bc54 100644
>>> --- a/gcc/ipa-icf.h
>>> +++ b/gcc/ipa-icf.h
>>> @@ -29,7 +29,8 @@ class congruence_class
>>>   {
>>>   public:
>>>     /* Congruence class constructor for a new class with _ID.  */
>>> -  congruence_class (unsigned int _id): in_worklist (false), id(_id)
>>> +  congruence_class (unsigned int _id): in_worklist (false), id(_id),
>>> +    sensitive_reference (false)
>>>     {
>>>     }
>>>
>>> @@ -54,6 +55,9 @@ public:
>>>
>>>     /* Global unique class identifier.  */
>>>     unsigned int id;
>>> +
>>> +  /* A function from the class contains a reference to another class.  */
>>> +  bool sensitive_reference;
>>>   };
>>>
>>>   /* Semantic item type enum.  */
>>> @@ -476,6 +480,10 @@ private:
>>>     /* Iterative congruence reduction function.  */
>>>     void process_cong_reduction (void);
>>>
>>> +  /* Identify congruence classes which have an address taken. These
>>> +     classes can be potentially been merged just as thunks.  */
>>> +  void identify_address_sensitive_classes (void);
>>> +
>>>     /* After reduction is done, we can declare all items in a group
>>>        to be equal. PREV_CLASS_COUNT is start number of classes
>>>        before reduction.  */
>>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> index 0c5a5a6..f25a251 100644
>>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> @@ -38,7 +38,7 @@ 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 "A function from the congruence class uses a sensitive reference." "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..66bcac5
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>>> @@ -0,0 +1,44 @@
>>> +/* { 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: 2" "icf"  } } */
>>> +/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf"  } } */
>>> +/* { dg-final { cleanup-ipa-dump "icf" } } */
>>> --
>>> 2.1.2
>>>
>>
>

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

On 02/13/2015 02:37 PM, Martin Liška wrote:
> On 02/12/2015 05:57 PM, Jan Hubicka wrote:
>>> From: mliska <mliska@suse.cz>V
>>> Date: Fri, 23 Jan 2015 14:58:36 +0100
>>> Subject: [PATCH] Fix PR ipa/64693.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2015-02-12  Martin Liska  <mliska@suse.cz>
>>>
>>>     PR ipa/64693
>>>     * ipa-icf.c (sem_item_optimizer::execute): Call identify_address_sensitive_classes.
>>>     (sem_item_optimizer::identify_address_sensitive_classes): New function.
>>>     (sem_item_optimizer::merge_classes): Handle cases where we merge a class
>>>     with a sensitive reference.
>>>     (congruence_class::dump): Add support for a new sensitive_reference flag.
>>>     * ipa-icf.h: Add new function.
>>>
>>> +
>>> +/* Identify congruence classes which have an address taken. These
>>> +   classes can be potentially been merged just as thunks.  */
>>
>> Hmm, I would expect someting like this
>>
>>   1) break out code in sem_function::merge which decides if function A and B
>>      can be identified or if thunk is needed
>>   2) when comparing references to verify congruence, actually check if merging
>>      would result in thunk and subdivide.
>>
>> In the following:
>>
>> void f1()
>> {
>> }
>> void f2()
>> {
>> }
>> void *a=&f1;
>> void *b=&f1;
>> void *c=&f2;
>> void *d=&f2;
>>
>> The code bellow seems to disable all merging, while we ought to be able to mere A,B
>> and C,D into two congruences.
>> Similarly we ought to be able to merge in a case F2 is a virtual function/ctor/dtor
>> where address can not be compared for equality.
>
> Hello.
>
> The code below works fine with this example:
> Semantic equality hit:d->c
> Semantic equality hit:b->a
> Semantic equality hit:f2->f1
>
> It just provides guard for following very special situation (cc1 LTO contains just 2 classes with this example),
> which is in gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c:
>
> Where we have functions in these classes:
> class 1:
>    foo,bar
>
> class 2:
>    callback1, callback2
>
> Where foo references callback1 and bar points to callback2. As callback{1,2} are in the same congruence class
> we can process merge operation of foo and bar. Unfortunately, address of callback{1,2} is taken and in this case
> we cannot merge foo and bar.
>
> As you said, to make things really correct, one should split class 1 according to these references.
> Question is if we want to do it for GCC 5.0 or prepare if for next release?
>
> I'm going to measure number of occurrences in Firefox and can make a decision.
> I like the idea of separating function in a class to candidates that need thunk creation and the rest. That will
> work seamlessly with your idea to include all functions that are just 'wrappers' for a function.
>
> Thanks for ideas,
> Martin
>

Hi.

As I expected, there are just 17 classes (mainly formed by pair of functions) that are marked
by the patch as reference_sensitive.

Martin

>>> +
>>> +void
>>> +sem_item_optimizer::identify_address_sensitive_classes (void)
>>> +{
>>> +  for (hash_table<congruence_class_group_hash>::iterator it = m_classes.begin ();
>>> +       it != m_classes.end (); ++it)
>>> +    for (unsigned i = 0; i < (*it)->classes.length (); i++)
>>> +      {
>>> +    congruence_class *cls = (*it)->classes[i];
>>> +
>>> +    if (cls->members.length () == 1)
>>> +      continue;
>>> +
>>> +    auto_vec <sem_item *> references;
>>> +    sem_item *source_node = cls->members[0];
>>> +    ipa_ref *r;
>>> +
>>> +    for (unsigned j = 0; j < source_node->node->num_references (); j++)
>>> +      {
>>> +        symtab_node *ref = source_node->node->iterate_reference (j, r)->referred;
>>> +        sem_item **symbol = m_symtab_node_map.get (ref);
>>> +        if (symbol)
>>> +          references.safe_push (*symbol);
>>> +      }
>>> +
>>> +    for (unsigned j = 1; j < cls->members.length (); j++)
>>> +      {
>>> +        source_node = cls->members[j];
>>> +
>>> +        unsigned l = 0;
>>> +        for (unsigned k = 0; k < source_node->node->num_references (); k++)
>>> +          {
>>> +        symtab_node *ref = source_node->node->iterate_reference (k, r)->referred;
>>> +        sem_item **symbol = m_symtab_node_map.get (ref);
>>> +        if (symbol)
>>> +          {
>>> +            if (l >= references.length () || references[l] != *symbol)
>>> +              {
>>> +            cls->sensitive_reference = true;
>>> +            break;
>>> +              }
>>> +
>>> +            l++;
>>> +          }
>>> +          }
>>> +
>>> +        if (cls->sensitive_reference)
>>> +          break;
>>> +      }
>>> +      }
>>> +}
>>> +
>>>   /* Debug function prints all informations about congruence classes.  */
>>>
>>>   void
>>> @@ -2374,6 +2430,15 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>>           continue;
>>>             }
>>>
>>> +        if (c->sensitive_reference)
>>> +          {
>>> +        if (dump_file)
>>> +          fprintf (dump_file, "A function from the congruence class "
>>> +               "uses a sensitive reference.\n");
>>> +
>>> +        continue;
>>> +          }
>>> +
>>>           if (dump_file && (dump_flags & TDF_DETAILS))
>>>             {
>>>           source->dump_to_file (dump_file);
>>> @@ -2390,8 +2455,10 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>>   void
>>>   congruence_class::dump (FILE *file, unsigned int indent) const
>>>   {
>>> -  FPRINTF_SPACES (file, indent, "class with id: %u, hash: %u, items: %u\n",
>>> -          id, members[0]->get_hash (), members.length ());
>>> +  FPRINTF_SPACES (file, indent,
>>> +          "class with id: %u, hash: %u, items: %u %s\n",
>>> +          id, members[0]->get_hash (), members.length (),
>>> +          sensitive_reference ? "sensitive_reference" : "");
>>>
>>>     FPUTS_SPACES (file, indent + 2, "");
>>>     for (unsigned i = 0; i < members.length (); i++)
>>> diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
>>> index adbedd6..db0bc54 100644
>>> --- a/gcc/ipa-icf.h
>>> +++ b/gcc/ipa-icf.h
>>> @@ -29,7 +29,8 @@ class congruence_class
>>>   {
>>>   public:
>>>     /* Congruence class constructor for a new class with _ID.  */
>>> -  congruence_class (unsigned int _id): in_worklist (false), id(_id)
>>> +  congruence_class (unsigned int _id): in_worklist (false), id(_id),
>>> +    sensitive_reference (false)
>>>     {
>>>     }
>>>
>>> @@ -54,6 +55,9 @@ public:
>>>
>>>     /* Global unique class identifier.  */
>>>     unsigned int id;
>>> +
>>> +  /* A function from the class contains a reference to another class.  */
>>> +  bool sensitive_reference;
>>>   };
>>>
>>>   /* Semantic item type enum.  */
>>> @@ -476,6 +480,10 @@ private:
>>>     /* Iterative congruence reduction function.  */
>>>     void process_cong_reduction (void);
>>>
>>> +  /* Identify congruence classes which have an address taken. These
>>> +     classes can be potentially been merged just as thunks.  */
>>> +  void identify_address_sensitive_classes (void);
>>> +
>>>     /* After reduction is done, we can declare all items in a group
>>>        to be equal. PREV_CLASS_COUNT is start number of classes
>>>        before reduction.  */
>>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> index 0c5a5a6..f25a251 100644
>>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>>> @@ -38,7 +38,7 @@ 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 "A function from the congruence class uses a sensitive reference." "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..66bcac5
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>>> @@ -0,0 +1,44 @@
>>> +/* { 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: 2" "icf"  } } */
>>> +/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf"  } } */
>>> +/* { dg-final { cleanup-ipa-dump "icf" } } */
>>> --
>>> 2.1.2
>>>
>>
>

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 7796 bytes --]

On 02/12/2015 05:57 PM, Jan Hubicka wrote:
>> From: mliska <mliska@suse.cz>V
>> Date: Fri, 23 Jan 2015 14:58:36 +0100
>> Subject: [PATCH] Fix PR ipa/64693.
>>
>> gcc/ChangeLog:
>>
>> 2015-02-12  Martin Liska  <mliska@suse.cz>
>>
>> 	PR ipa/64693
>> 	* ipa-icf.c (sem_item_optimizer::execute): Call identify_address_sensitive_classes.
>> 	(sem_item_optimizer::identify_address_sensitive_classes): New function.
>> 	(sem_item_optimizer::merge_classes): Handle cases where we merge a class
>> 	with a sensitive reference.
>> 	(congruence_class::dump): Add support for a new sensitive_reference flag.
>> 	* ipa-icf.h: Add new function.
>>
>> +
>> +/* Identify congruence classes which have an address taken. These
>> +   classes can be potentially been merged just as thunks.  */
>
> Hmm, I would expect someting like this
>
>   1) break out code in sem_function::merge which decides if function A and B
>      can be identified or if thunk is needed
>   2) when comparing references to verify congruence, actually check if merging
>      would result in thunk and subdivide.
>
> In the following:
>
> void f1()
> {
> }
> void f2()
> {
> }
> void *a=&f1;
> void *b=&f1;
> void *c=&f2;
> void *d=&f2;
>
> The code bellow seems to disable all merging, while we ought to be able to mere A,B
> and C,D into two congruences.
> Similarly we ought to be able to merge in a case F2 is a virtual function/ctor/dtor
> where address can not be compared for equality.
>> +
>> +void
>> +sem_item_optimizer::identify_address_sensitive_classes (void)
>> +{
>> +  for (hash_table<congruence_class_group_hash>::iterator it = m_classes.begin ();
>> +       it != m_classes.end (); ++it)
>> +    for (unsigned i = 0; i < (*it)->classes.length (); i++)
>> +      {
>> +	congruence_class *cls = (*it)->classes[i];
>> +
>> +	if (cls->members.length () == 1)
>> +	  continue;
>> +
>> +	auto_vec <sem_item *> references;
>> +	sem_item *source_node = cls->members[0];
>> +	ipa_ref *r;
>> +
>> +	for (unsigned j = 0; j < source_node->node->num_references (); j++)
>> +	  {
>> +	    symtab_node *ref = source_node->node->iterate_reference (j, r)->referred;
>> +	    sem_item **symbol = m_symtab_node_map.get (ref);
>> +	    if (symbol)
>> +	      references.safe_push (*symbol);
>> +	  }
>> +
>> +	for (unsigned j = 1; j < cls->members.length (); j++)
>> +	  {
>> +	    source_node = cls->members[j];
>> +
>> +	    unsigned l = 0;
>> +	    for (unsigned k = 0; k < source_node->node->num_references (); k++)
>> +	      {
>> +		symtab_node *ref = source_node->node->iterate_reference (k, r)->referred;
>> +		sem_item **symbol = m_symtab_node_map.get (ref);
>> +		if (symbol)
>> +		  {
>> +		    if (l >= references.length () || references[l] != *symbol)
>> +		      {
>> +			cls->sensitive_reference = true;
>> +			break;
>> +		      }
>> +
>> +		    l++;
>> +		  }
>> +	      }
>> +
>> +	    if (cls->sensitive_reference)
>> +	      break;
>> +	  }
>> +      }
>> +}
>> +
>>   /* Debug function prints all informations about congruence classes.  */
>>
>>   void
>> @@ -2374,6 +2430,15 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>   		continue;
>>   	      }
>>
>> +	    if (c->sensitive_reference)
>> +	      {
>> +		if (dump_file)
>> +		  fprintf (dump_file, "A function from the congruence class "
>> +			   "uses a sensitive reference.\n");
>> +
>> +		continue;
>> +	      }
>> +
>>   	    if (dump_file && (dump_flags & TDF_DETAILS))
>>   	      {
>>   		source->dump_to_file (dump_file);
>> @@ -2390,8 +2455,10 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>   void
>>   congruence_class::dump (FILE *file, unsigned int indent) const
>>   {
>> -  FPRINTF_SPACES (file, indent, "class with id: %u, hash: %u, items: %u\n",
>> -		  id, members[0]->get_hash (), members.length ());
>> +  FPRINTF_SPACES (file, indent,
>> +		  "class with id: %u, hash: %u, items: %u %s\n",
>> +		  id, members[0]->get_hash (), members.length (),
>> +		  sensitive_reference ? "sensitive_reference" : "");
>>
>>     FPUTS_SPACES (file, indent + 2, "");
>>     for (unsigned i = 0; i < members.length (); i++)
>> diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
>> index adbedd6..db0bc54 100644
>> --- a/gcc/ipa-icf.h
>> +++ b/gcc/ipa-icf.h
>> @@ -29,7 +29,8 @@ class congruence_class
>>   {
>>   public:
>>     /* Congruence class constructor for a new class with _ID.  */
>> -  congruence_class (unsigned int _id): in_worklist (false), id(_id)
>> +  congruence_class (unsigned int _id): in_worklist (false), id(_id),
>> +    sensitive_reference (false)
>>     {
>>     }
>>
>> @@ -54,6 +55,9 @@ public:
>>
>>     /* Global unique class identifier.  */
>>     unsigned int id;
>> +
>> +  /* A function from the class contains a reference to another class.  */
>> +  bool sensitive_reference;
>>   };
>>
>>   /* Semantic item type enum.  */
>> @@ -476,6 +480,10 @@ private:
>>     /* Iterative congruence reduction function.  */
>>     void process_cong_reduction (void);
>>
>> +  /* Identify congruence classes which have an address taken. These
>> +     classes can be potentially been merged just as thunks.  */
>> +  void identify_address_sensitive_classes (void);
>> +
>>     /* After reduction is done, we can declare all items in a group
>>        to be equal. PREV_CLASS_COUNT is start number of classes
>>        before reduction.  */
>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> index 0c5a5a6..f25a251 100644
>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> @@ -38,7 +38,7 @@ 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 "A function from the congruence class uses a sensitive reference." "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..66bcac5
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>> @@ -0,0 +1,44 @@
>> +/* { 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: 2" "icf"  } } */
>> +/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf"  } } */
>> +/* { dg-final { cleanup-ipa-dump "icf" } } */
>> --
>> 2.1.2
>>
>

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



[-- Attachment #2: 0001-Fix-PR-ipa-64693.patch --]
[-- Type: text/x-patch, Size: 10387 bytes --]

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.
---
 gcc/ipa-icf.c                         | 97 ++++++++++++++++++++++++++++++++++-
 gcc/ipa-icf.h                         | 72 +++++++++++++++++++++++++-
 gcc/ipa-ref.h                         | 13 +++++
 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 ++++++++++++++++
 6 files changed, 223 insertions(+), 6 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 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.  */
+
+unsigned
+sem_item_optimizer::subdivide_classes_by_addr_references ()
+{
+  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->has_member_with_addr_ref)
+	    continue;
+
+	  if (c->members.length() > 1)
+	    {
+	      hash_map <addr_refs_collection *, vec <sem_item *>, addr_refs_hashmap_traits> split_map;
+
+	      for (unsigned j = 0; j < c->members.length (); j++)
+	        {
+		  sem_item *source_node = c->members[j];
+
+		  addr_refs_collection *collection = new addr_refs_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 <addr_refs_collection *, vec <sem_item *>, addr_refs_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
@@ -2258,8 +2339,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 ();
+
+    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..26facc4 100644
--- a/gcc/ipa-icf.h
+++ b/gcc/ipa-icf.h
@@ -29,7 +29,8 @@ class congruence_class
 {
 public:
   /* Congruence class constructor for a new class with _ID.  */
-  congruence_class (unsigned int _id): in_worklist (false), id(_id)
+  congruence_class (unsigned int _id): in_worklist (false), id(_id),
+    has_member_with_addr_ref (false)
   {
   }
 
@@ -54,6 +55,9 @@ public:
 
   /* Global unique class identifier.  */
   unsigned int id;
+
+  /* Identify if a member of this class has an address reference.  */
+  bool has_member_with_addr_ref;
 };
 
 /* Semantic item type enum.  */
@@ -63,6 +67,60 @@ enum sem_item_type
   VAR
 };
 
+/* 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);
+      }
+  }
+
+  /* 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 ();
+  }
+
+  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;
+  }
+};
+
+
 /* Semantic item usage pair.  */
 class sem_usage_pair
 {
@@ -467,6 +525,12 @@ private:
      classes. If IN_WPA, fast equality function is invoked.  */
   void subdivide_classes_by_equality (bool in_wpa = false);
 
+  /* 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 subdivide_classes_by_addr_references ();
+
   /* Debug function prints all informations about congruence classes.  */
   void dump_cong_classes (void);
 
@@ -487,6 +551,12 @@ private:
   /* Pops a class from worklist. */
   congruence_class *worklist_pop ();
 
+  /* Return true if worklist is empty.  */
+  bool worklist_empty ()
+  {
+    return worklist.empty ();
+  }
+
   /* Every usage of a congruence class CLS is a candidate that can split the
      collection of classes. Bitmap stack BMSTACK is used for bitmap
      allocation.  */
diff --git a/gcc/ipa-ref.h b/gcc/ipa-ref.h
index aea7f4c..b2ed801 100644
--- a/gcc/ipa-ref.h
+++ b/gcc/ipa-ref.h
@@ -112,6 +112,19 @@ public:
     return first_alias ();
   }
 
+  /* Return true if there's any address reference.  */
+  bool inline has_addr_ref_p (void)
+  {
+    if (!vec_safe_length (references))
+      return 0;
+
+     for(unsigned i = 0; i < references->length (); i++)
+       if ((*references)[i].use == IPA_REF_ADDR)
+	 return true;
+
+    return false;
+  }
+
   /* Clear reference list.  */
   void clear (void)
   {
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

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 7806 bytes --]

On 02/12/2015 05:57 PM, Jan Hubicka wrote:
>> From: mliska <mliska@suse.cz>V
>> Date: Fri, 23 Jan 2015 14:58:36 +0100
>> Subject: [PATCH] Fix PR ipa/64693.
>>
>> gcc/ChangeLog:
>>
>> 2015-02-12  Martin Liska  <mliska@suse.cz>
>>
>> 	PR ipa/64693
>> 	* ipa-icf.c (sem_item_optimizer::execute): Call identify_address_sensitive_classes.
>> 	(sem_item_optimizer::identify_address_sensitive_classes): New function.
>> 	(sem_item_optimizer::merge_classes): Handle cases where we merge a class
>> 	with a sensitive reference.
>> 	(congruence_class::dump): Add support for a new sensitive_reference flag.
>> 	* ipa-icf.h: Add new function.
>>
>> +
>> +/* Identify congruence classes which have an address taken. These
>> +   classes can be potentially been merged just as thunks.  */
>
> Hmm, I would expect someting like this
>
>   1) break out code in sem_function::merge which decides if function A and B
>      can be identified or if thunk is needed
>   2) when comparing references to verify congruence, actually check if merging
>      would result in thunk and subdivide.
>
> In the following:
>
> void f1()
> {
> }
> void f2()
> {
> }
> void *a=&f1;
> void *b=&f1;
> void *c=&f2;
> void *d=&f2;
>
> The code bellow seems to disable all merging, while we ought to be able to mere A,B
> and C,D into two congruences.
> Similarly we ought to be able to merge in a case F2 is a virtual function/ctor/dtor
> where address can not be compared for equality.
>> +
>> +void
>> +sem_item_optimizer::identify_address_sensitive_classes (void)
>> +{
>> +  for (hash_table<congruence_class_group_hash>::iterator it = m_classes.begin ();
>> +       it != m_classes.end (); ++it)
>> +    for (unsigned i = 0; i < (*it)->classes.length (); i++)
>> +      {
>> +	congruence_class *cls = (*it)->classes[i];
>> +
>> +	if (cls->members.length () == 1)
>> +	  continue;
>> +
>> +	auto_vec <sem_item *> references;
>> +	sem_item *source_node = cls->members[0];
>> +	ipa_ref *r;
>> +
>> +	for (unsigned j = 0; j < source_node->node->num_references (); j++)
>> +	  {
>> +	    symtab_node *ref = source_node->node->iterate_reference (j, r)->referred;
>> +	    sem_item **symbol = m_symtab_node_map.get (ref);
>> +	    if (symbol)
>> +	      references.safe_push (*symbol);
>> +	  }
>> +
>> +	for (unsigned j = 1; j < cls->members.length (); j++)
>> +	  {
>> +	    source_node = cls->members[j];
>> +
>> +	    unsigned l = 0;
>> +	    for (unsigned k = 0; k < source_node->node->num_references (); k++)
>> +	      {
>> +		symtab_node *ref = source_node->node->iterate_reference (k, r)->referred;
>> +		sem_item **symbol = m_symtab_node_map.get (ref);
>> +		if (symbol)
>> +		  {
>> +		    if (l >= references.length () || references[l] != *symbol)
>> +		      {
>> +			cls->sensitive_reference = true;
>> +			break;
>> +		      }
>> +
>> +		    l++;
>> +		  }
>> +	      }
>> +
>> +	    if (cls->sensitive_reference)
>> +	      break;
>> +	  }
>> +      }
>> +}
>> +
>>   /* Debug function prints all informations about congruence classes.  */
>>
>>   void
>> @@ -2374,6 +2430,15 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>   		continue;
>>   	      }
>>
>> +	    if (c->sensitive_reference)
>> +	      {
>> +		if (dump_file)
>> +		  fprintf (dump_file, "A function from the congruence class "
>> +			   "uses a sensitive reference.\n");
>> +
>> +		continue;
>> +	      }
>> +
>>   	    if (dump_file && (dump_flags & TDF_DETAILS))
>>   	      {
>>   		source->dump_to_file (dump_file);
>> @@ -2390,8 +2455,10 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count)
>>   void
>>   congruence_class::dump (FILE *file, unsigned int indent) const
>>   {
>> -  FPRINTF_SPACES (file, indent, "class with id: %u, hash: %u, items: %u\n",
>> -		  id, members[0]->get_hash (), members.length ());
>> +  FPRINTF_SPACES (file, indent,
>> +		  "class with id: %u, hash: %u, items: %u %s\n",
>> +		  id, members[0]->get_hash (), members.length (),
>> +		  sensitive_reference ? "sensitive_reference" : "");
>>
>>     FPUTS_SPACES (file, indent + 2, "");
>>     for (unsigned i = 0; i < members.length (); i++)
>> diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
>> index adbedd6..db0bc54 100644
>> --- a/gcc/ipa-icf.h
>> +++ b/gcc/ipa-icf.h
>> @@ -29,7 +29,8 @@ class congruence_class
>>   {
>>   public:
>>     /* Congruence class constructor for a new class with _ID.  */
>> -  congruence_class (unsigned int _id): in_worklist (false), id(_id)
>> +  congruence_class (unsigned int _id): in_worklist (false), id(_id),
>> +    sensitive_reference (false)
>>     {
>>     }
>>
>> @@ -54,6 +55,9 @@ public:
>>
>>     /* Global unique class identifier.  */
>>     unsigned int id;
>> +
>> +  /* A function from the class contains a reference to another class.  */
>> +  bool sensitive_reference;
>>   };
>>
>>   /* Semantic item type enum.  */
>> @@ -476,6 +480,10 @@ private:
>>     /* Iterative congruence reduction function.  */
>>     void process_cong_reduction (void);
>>
>> +  /* Identify congruence classes which have an address taken. These
>> +     classes can be potentially been merged just as thunks.  */
>> +  void identify_address_sensitive_classes (void);
>> +
>>     /* After reduction is done, we can declare all items in a group
>>        to be equal. PREV_CLASS_COUNT is start number of classes
>>        before reduction.  */
>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> index 0c5a5a6..f25a251 100644
>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> @@ -38,7 +38,7 @@ 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 "A function from the congruence class uses a sensitive reference." "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..66bcac5
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>> @@ -0,0 +1,44 @@
>> +/* { 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: 2" "icf"  } } */
>> +/* { dg-final { scan-ipa-dump "A function from the congruence class uses a sensitive reference." "icf"  } } */
>> +/* { dg-final { cleanup-ipa-dump "icf" } } */
>> --
>> 2.1.2
>>
>

Hi.

This is second part which introduces better variable handling. Since readonly variable flag
identification can identify new candidates, ICF should filter out non-readonly variables in
execute phase.

Ready for trunk?
Thanks,
Martin

[-- Attachment #2: 0002-Fix-missed-optimization-for-vars-not-marked-by-const.patch --]
[-- Type: text/x-patch, Size: 2629 bytes --]

From a18a4840d14b1c0d35a9e4387daae29f5e8c906c Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Fri, 20 Feb 2015 11:15:37 +0100
Subject: [PATCH 2/2] Fix missed optimization for vars not marked by const.

gcc/testsuite/ChangeLog:

2015-02-20  Martin Liska  <mliska@suse.cz>

	* gcc.dg/ipa/ipa-icf-35.c: New test.

gcc/ChangeLog:

2015-02-20  Martin Liska  <mliska@suse.cz>

	* ipa-icf.c (sem_variable::parse): Ignore readonly flag that
	should be evaluated in driver.
	(sem_item_optimizer::filter_removed_items): Filter out
	non-readonly variables.
---
 gcc/ipa-icf.c                         | 13 ++++++++-----
 gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 859b9d1..5973b2f 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1228,10 +1228,6 @@ sem_variable::parse (varpool_node *node, bitmap_obstack *stack)
 {
   tree decl = node->decl;
 
-  bool readonly = TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl);
-  if (!readonly)
-    return NULL;
-
   bool can_handle = DECL_VIRTUAL_P (decl)
 		    || flag_merge_constants >= 2
 		    || (!TREE_ADDRESSABLE (decl) && !node->externally_visible);
@@ -1697,7 +1693,14 @@ sem_item_optimizer::filter_removed_items (void)
 	  if (!flag_ipa_icf_variables)
 	    remove_item (item);
 	  else
-	    filtered.safe_push (item);
+	    {
+	      /* Filter out non-readonly variables.  */
+	      tree decl = item->decl;
+	      if (TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl))
+		filtered.safe_push (item);
+	      else
+		remove_item (item);
+	    }
         }
     }
 
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c
new file mode 100644
index 0000000..95d247e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c
@@ -0,0 +1,34 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf"  } */
+
+#include <stdlib.h>
+#include <assert.h>
+
+void f1()
+{
+}
+
+void f2()
+{
+}
+
+static void (*a)(void)=&f1;
+static void (*b)(void)=&f1;
+static void (*c)(void)=&f2;
+static void (*d)(void)=&f2;
+
+int main()
+{
+  a();
+  b();
+  c();
+  d();
+
+  return 0;
+}
+
+/* { dg-final { scan-ipa-dump "Equal symbols: 3" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:f2->f1" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:d->c" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:b->a" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2

Re: [PATCH] Fix for PR ipa/64693

[Jan Hubicka]

> 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

Re: [PATCH] Fix for PR ipa/64693

[Jan Hubicka]

> 
> Hi.
> 
> This is second part which introduces better variable handling. Since readonly variable flag
> identification can identify new candidates, ICF should filter out non-readonly variables in
> execute phase.
> 
> Ready for trunk?
> Thanks,
> Martin

> >From a18a4840d14b1c0d35a9e4387daae29f5e8c906c Mon Sep 17 00:00:00 2001
> From: mliska <mliska@suse.cz>
> Date: Fri, 20 Feb 2015 11:15:37 +0100
> Subject: [PATCH 2/2] Fix missed optimization for vars not marked by const.
> 
> gcc/testsuite/ChangeLog:
> 
> 2015-02-20  Martin Liska  <mliska@suse.cz>
> 
> 	* gcc.dg/ipa/ipa-icf-35.c: New test.
> 
> gcc/ChangeLog:
> 
> 2015-02-20  Martin Liska  <mliska@suse.cz>
> 
> 	* ipa-icf.c (sem_variable::parse): Ignore readonly flag that
> 	should be evaluated in driver.
> 	(sem_item_optimizer::filter_removed_items): Filter out
> 	non-readonly variables.
> ---
>  gcc/ipa-icf.c                         | 13 ++++++++-----
>  gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 5 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c
> 
> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index 859b9d1..5973b2f 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -1228,10 +1228,6 @@ sem_variable::parse (varpool_node *node, bitmap_obstack *stack)
>  {
>    tree decl = node->decl;
>  
> -  bool readonly = TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl);
> -  if (!readonly)
> -    return NULL;
> -
>    bool can_handle = DECL_VIRTUAL_P (decl)
>  		    || flag_merge_constants >= 2
>  		    || (!TREE_ADDRESSABLE (decl) && !node->externally_visible);

You want to remove can_handle test, too, because function may become static as effect
of LTO.

> @@ -1697,7 +1693,14 @@ sem_item_optimizer::filter_removed_items (void)
>  	  if (!flag_ipa_icf_variables)
>  	    remove_item (item);
>  	  else
> -	    filtered.safe_push (item);
> +	    {
> +	      /* Filter out non-readonly variables.  */
> +	      tree decl = item->decl;
> +	      if (TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl))

Instead of readonly && can_handle you are doing now:
  bool readonly = TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl);  
  bool can_handle = DECL_VIRTUAL_P (decl)                                       
                    || flag_merge_constants >= 2                                
                    || (!TREE_ADDRESSABLE (decl) && !node->externally_visible); 

please just test TREE_ADDRESSABLE (again do it late because var may become !TREE_ADDRESSABLE
as a result of optimiziation) and varpool_node::ctor_useable_for_folding_p

OK with this change.

Honza

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 5366 bytes --]

On 02/20/2015 07:39 PM, Jan Hubicka wrote:
>> 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
>

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

[-- Attachment #2: 0001-Fix-PR-ipa-64693.patch --]
[-- Type: text/x-patch, Size: 11518 bytes --]

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.
---
 gcc/ipa-icf.c                         | 130 ++++++++++++++++++++++++++++++++--
 gcc/ipa-icf.h                         |  85 ++++++++++++++++++++++
 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 +++++++++++
 5 files changed, 255 insertions(+), 7 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 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)
+{
+  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);
+
+      if (ref->referred->get_availability () <= AVAIL_INTERPOSABLE)
+	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):
@@ -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 ()));
 
@@ -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 ()
+{
+  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 <addr_refs_collection *, vec <sem_item *>, addr_refs_hashmap_traits> split_map;
+
+	      for (unsigned j = 0; j < c->members.length (); j++)
+	        {
+		  sem_item *source_node = c->members[j];
+
+		  addr_refs_collection *collection = new addr_refs_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 <addr_refs_collection *, vec <sem_item *>, addr_refs_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
@@ -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 ();
+
+    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..dd016f3 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 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;
+};
+
+/* 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 ());
+
+    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 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;
+
+    return true;
+  }
+};
+
+
 /* Semantic item usage pair.  */
 class sem_usage_pair
 {
@@ -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)
+  {
+    return !DECL_VIRTUAL_P (decl)
+	   && (TREE_CODE (decl) != FUNCTION_DECL
+	       || (!DECL_CXX_CONSTRUCTOR_P (decl)
+		   && !DECL_CXX_DESTRUCTOR_P (decl)));
+  }
+
   /* Return true if target supports alias symbols.  */
   bool target_supports_symbol_aliases_p (void);
 
@@ -467,6 +540,12 @@ private:
      classes. If IN_WPA, fast equality function is invoked.  */
   void subdivide_classes_by_equality (bool in_wpa = false);
 
+  /* 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 subdivide_classes_by_addr_references ();
+
   /* Debug function prints all informations about congruence classes.  */
   void dump_cong_classes (void);
 
@@ -487,6 +566,12 @@ private:
   /* Pops a class from worklist. */
   congruence_class *worklist_pop ();
 
+  /* Return true if worklist is empty.  */
+  bool worklist_empty ()
+  {
+    return worklist.empty ();
+  }
+
   /* Every usage of a congruence class CLS is a candidate that can split the
      collection of classes. Bitmap stack BMSTACK is used for bitmap
      allocation.  */
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

Re: [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

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 6274 bytes --]

On 02/25/2015 06:00 PM, Jan Hubicka wrote:
>> 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.

Agree with that, so I've taken the chunk which defines 'address_matters_p' from your big patch.

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

Ok, I'm going to write it.

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

Renamed.

>> @@ -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 ;)

You are right, loop is removed.

>> +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.

Yes.

Updated patch is attached, please read it once more. Tests have been just started.

Thanks,
Martin

>
> OK with these changes.
> Honza
>


[-- Attachment #2: 0001-Fix-PR-ipa-64693.patch --]
[-- Type: text/x-patch, Size: 13921 bytes --]

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.
---
 gcc/cgraph.h                          |   4 ++
 gcc/ipa-icf.c                         | 121 ++++++++++++++++++++++++++++++++++
 gcc/ipa-icf.h                         |  86 ++++++++++++++++++++++++
 gcc/ipa-visibility.c                  |  21 ------
 gcc/symtab.c                          |  48 ++++++++++++++
 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, 303 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index ec3cccd..531d149 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -337,6 +337,10 @@ public:
      return 2 otherwise.   */
   int equal_address_to (symtab_node *s2);
 
+  /* Return true if symbol's address may possibly be compared to other
+     symbol's address.  */
+  bool address_matters_p ();
+
   /* Return symbol table node associated with DECL, if any,
      and NULL otherwise.  */
   static inline symtab_node *get (const_tree decl)
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index e1af8bf..b8bfd96 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.  */
+
+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->use == IPA_REF_ADDR && ref->referred->address_matters_p ()
+	  && !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);
+    }
+
+  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 +2001,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 +2368,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..a00872e 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
 {
@@ -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)
+  {
+    return !DECL_VIRTUAL_P (decl)
+	   && (TREE_CODE (decl) != FUNCTION_DECL
+	       || (!DECL_CXX_CONSTRUCTOR_P (decl)
+		   && !DECL_CXX_DESTRUCTOR_P (decl)));
+  }
+
   /* Return true if target supports alias symbols.  */
   bool target_supports_symbol_aliases_p (void);
 
@@ -467,6 +540,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);
 
@@ -487,6 +567,12 @@ private:
   /* Pops a class from worklist. */
   congruence_class *worklist_pop ();
 
+  /* Return true if worklist is empty.  */
+  bool worklist_empty ()
+  {
+    return worklist.empty ();
+  }
+
   /* Every usage of a congruence class CLS is a candidate that can split the
      collection of classes. Bitmap stack BMSTACK is used for bitmap
      allocation.  */
diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
index 9247e29..5b0c74c 100644
--- a/gcc/ipa-visibility.c
+++ b/gcc/ipa-visibility.c
@@ -129,27 +129,6 @@ cgraph_node::local_p (void)
 					
 }
 
-/* Return true when there is a reference to node and it is not vtable.  */
-
-bool
-symtab_node::address_taken_from_non_vtable_p (void)
-{
-  int i;
-  struct ipa_ref *ref = NULL;
-
-  for (i = 0; iterate_referring (i, ref); i++)
-    if (ref->use == IPA_REF_ADDR)
-      {
-	varpool_node *node;
-	if (is_a <cgraph_node *> (ref->referring))
-	  return true;
-	node = dyn_cast <varpool_node *> (ref->referring);
-	if (!DECL_VIRTUAL_P (node->decl))
-	  return true;
-      }
-  return false;
-}
-
 /* A helper for comdat_can_be_unshared_p.  */
 
 static bool
diff --git a/gcc/symtab.c b/gcc/symtab.c
index 7a70b10..463bd28 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -1862,3 +1862,51 @@ symtab_node::call_for_symbol_and_aliases_1 (bool (*callback) (symtab_node *,
     }
   return false;
 }
+
+/* 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;
+}
+
+/* Return true when there is a reference to node and it is not vtable.  */
+
+bool
+symtab_node::address_taken_from_non_vtable_p (void)
+{
+  int i;
+  struct ipa_ref *ref = NULL;
+
+  for (i = 0; iterate_referring (i, ref); i++)
+    if (ref->use == IPA_REF_ADDR)
+      {
+	varpool_node *node;
+	if (is_a <cgraph_node *> (ref->referring))
+	  return true;
+	node = dyn_cast <varpool_node *> (ref->referring);
+	if (!DECL_VIRTUAL_P (node->decl))
+	  return true;
+      }
+  return false;
+}
+
+/* Return true if symbol's address may possibly be compared to other
+   symbol's address.  */
+
+bool
+symtab_node::address_matters_p ()
+{
+  gcc_assert (!alias);
+  return call_for_symbol_and_aliases (address_matters_1, NULL, true);
+}
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

Re: [PATCH] Fix for PR ipa/64693

[Jan Hubicka]

> 

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

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 3946 bytes --]

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

[-- Attachment #2: 0001-Fix-PR-ipa-64693.patch --]
[-- Type: text/x-patch, Size: 12134 bytes --]

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

Re: [PATCH] Fix for PR ipa/64693

[Jan Hubicka]

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.

OK
Honza

Re: [PATCH] Fix for PR ipa/64693

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

On 02/26/2015 07:21 PM, Jan Hubicka wrote:
> 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.
>
> OK
> Honza
>

Hi.

There's one missing vector comparison. Fix is obvious, ready for trunk?

Thanks,
Martin

[-- Attachment #2: 0001-Fix-missing-condition-in-symbol_compare_hashmap_trai.patch --]
[-- Type: text/x-patch, Size: 1026 bytes --]

From 3d03fb28ec21b6ed30d5179bd70aba79d246cd26 Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Fri, 27 Feb 2015 16:35:31 +0100
Subject: [PATCH] Fix missing condition in symbol_compare_hashmap_traits.

gcc/ChangeLog:

2015-02-27  Martin Liska  <mliska@suse.cz>

	* ipa-icf.h (struct symbol_compare_hashmap_traits): Add missing
	vector length condition.
---
 gcc/ipa-icf.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
index 9e76239..077267c 100644
--- a/gcc/ipa-icf.h
+++ b/gcc/ipa-icf.h
@@ -110,7 +110,8 @@ struct symbol_compare_hashmap_traits: default_hashmap_traits
   equal_keys (const symbol_compare_collection *a,
 	      const symbol_compare_collection *b)
   {
-    if (a->m_references.length () != b->m_references.length ())
+    if (a->m_references.length () != b->m_references.length ()
+	|| a->m_interposables.length () != b->m_interposables.length ())
       return false;
 
     for (unsigned i = 0; i < a->m_references.length (); i++)
-- 
2.1.2

Re: [PATCH] Fix for PR ipa/64693

[Jan Hubicka]

> Hi.
> 
> There's one missing vector comparison. Fix is obvious, ready for trunk?
> 
> Thanks,
> Martin

> >From 3d03fb28ec21b6ed30d5179bd70aba79d246cd26 Mon Sep 17 00:00:00 2001
> From: mliska <mliska@suse.cz>
> Date: Fri, 27 Feb 2015 16:35:31 +0100
> Subject: [PATCH] Fix missing condition in symbol_compare_hashmap_traits.
> 
> gcc/ChangeLog:
> 
> 2015-02-27  Martin Liska  <mliska@suse.cz>
> 
> 	* ipa-icf.h (struct symbol_compare_hashmap_traits): Add missing
> 	vector length condition.
OK
Honza
> ---
>  gcc/ipa-icf.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
> index 9e76239..077267c 100644
> --- a/gcc/ipa-icf.h
> +++ b/gcc/ipa-icf.h
> @@ -110,7 +110,8 @@ struct symbol_compare_hashmap_traits: default_hashmap_traits
>    equal_keys (const symbol_compare_collection *a,
>  	      const symbol_compare_collection *b)
>    {
> -    if (a->m_references.length () != b->m_references.length ())
> +    if (a->m_references.length () != b->m_references.length ()
> +	|| a->m_interposables.length () != b->m_interposables.length ())
>        return false;
>  
>      for (unsigned i = 0; i < a->m_references.length (); i++)
> -- 
> 2.1.2
>