diff mbox

Fix for PR ipa/64693

Message ID 54E730D4.1010403@suse.cz
State New
Headers show

Commit Message

Martin Liška Feb. 20, 2015, 1:04 p.m. UTC 
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

Comments

Jan Hubicka Feb. 20, 2015, 6:43 p.m. UTC | #1 
> 
> 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
diff mbox

Patch

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