diff mbox

Fix memory leak in IPA passes

Message ID 552CCC4E.7090208@suse.cz
State New
Headers show

Commit Message

Martin Liška April 14, 2015, 8:14 a.m. UTC 
Hello.

As originally reported by Andi Kleen, following patch fix memory leaks that can
be seen in IPA ICF and IPA CP.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Moreover, with patch applied, we can build Firefox and Linux kernel on ppc64le-linux-gnu.

Ready for both trunk and 5 branch?
Thanks,
Martin

Comments

Jakub Jelinek April 14, 2015, 8:18 a.m. UTC | #1 
On Tue, Apr 14, 2015 at 10:14:06AM +0200, Martin Liška wrote:
> As originally reported by Andi Kleen, following patch fix memory leaks that can
> be seen in IPA ICF and IPA CP.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> Moreover, with patch applied, we can build Firefox and Linux kernel on ppc64le-linux-gnu.
> 
> Ready for both trunk and 5 branch?

If Honza acks this, I'd prefer if it went to 5 branch only after 5.1 is
released.

	Jakub
Martin Liška April 14, 2015, 8:23 a.m. UTC | #2 
On 04/14/2015 10:18 AM, Jakub Jelinek wrote:
> On Tue, Apr 14, 2015 at 10:14:06AM +0200, Martin Liška wrote:
>> As originally reported by Andi Kleen, following patch fix memory leaks that can
>> be seen in IPA ICF and IPA CP.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>> Moreover, with patch applied, we can build Firefox and Linux kernel on ppc64le-linux-gnu.
>>
>> Ready for both trunk and 5 branch?
>
> If Honza acks this, I'd prefer if it went to 5 branch only after 5.1 is
> released.
>
> 	Jakub
>

Hello

I have no problem with that, memory leaks are quite small I guess.

Martin
Jan Hubicka April 14, 2015, 1:45 p.m. UTC | #3 
> 
> 2015-04-13  Martin Liska  <mliska@suse.cz>
> 
> 	* ipa-cp.c (ipcp_driver): Release prev_edge_clone.
> 	* ipa-icf.c (sem_item_optimizer::subdivide_classes_by_sensitive_refs):
> 	Release symbol_compare_collection.
> 	* ipa-reference.c: Add TODO that a vector should be released.
> ---
>  gcc/ipa-cp.c        |  1 +
>  gcc/ipa-icf.c       | 26 ++++++++++++++++++++------
>  gcc/ipa-reference.c |  1 +
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index bfe4821..3824029 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -4493,6 +4493,7 @@ ipcp_driver (void)
>    /* Free all IPCP structures.  */
>    free_toporder_info (&topo);
>    next_edge_clone.release ();
> +  prev_edge_clone.release ();
>    symtab->remove_edge_removal_hook (edge_removal_hook_holder);
>    symtab->remove_edge_duplication_hook (edge_duplication_hook_holder);
>    ipa_free_all_structures_after_ipa_cp ();

I already fixed this one on mainline.

> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index b902373..a72ac2e 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -2712,6 +2712,9 @@ sem_item_optimizer::subdivide_classes_by_equality (bool in_wpa)
>  unsigned
>  sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>  {
> +  typedef hash_map <symbol_compare_collection *, vec <sem_item *>,
> +    symbol_compare_hashmap_traits> subdivide_hash_map;
> +
>    unsigned newly_created_classes = 0;
>  
>    for (hash_table <congruence_class_group_hash>::iterator it = m_classes.begin ();
> @@ -2726,8 +2729,7 @@ sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>  
>  	  if (c->members.length() > 1)
>  	    {
> -	      hash_map <symbol_compare_collection *, vec <sem_item *>,
> -		symbol_compare_hashmap_traits> split_map;
> +	      subdivide_hash_map split_map;
>  
>  	      for (unsigned j = 0; j < c->members.length (); j++)
>  	        {
> @@ -2735,10 +2737,15 @@ sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>  
>  		  symbol_compare_collection *collection = new symbol_compare_collection (source_node->node);
>  
> -		  vec <sem_item *> *slot = &split_map.get_or_insert (collection);
> +		  bool existed;
> +		  vec <sem_item *> *slot = &split_map.get_or_insert (collection,
> +								     &existed);
>  		  gcc_checking_assert (slot);
>  
>  		  slot->safe_push (source_node);
> +
> +		  if (existed)
> +		    delete collection;
>  	        }
>  
>  	       /* If the map contains more than one key, we have to split the map
> @@ -2747,9 +2754,8 @@ sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>  	        {
>  		  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)
> +		  for (subdivide_hash_map::iterator it2 = split_map.begin ();
> +		       it2 != split_map.end (); ++it2)
>  		    {
>  		      congruence_class *new_cls;
>  		      new_cls = new congruence_class (class_id++);
> @@ -2772,6 +2778,14 @@ sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>  		        }
>  		    }
>  		}
> +
> +	      /* Release memory.  */
> +	      for (subdivide_hash_map::iterator it2 = split_map.begin ();
> +		   it2 != split_map.end (); ++it2)
> +		{
> +		  delete (*it2).first;
> +		  (*it2).second.release ();
> +		}
>  	    }
>  	  }
>  

This is OK.

> diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
> index 219a9b3..a420cb2 100644
> --- a/gcc/ipa-reference.c
> +++ b/gcc/ipa-reference.c
> @@ -150,6 +150,7 @@ static struct cgraph_node_hook_list *node_removal_hook_holder;
>     Indexed by UID of call graph nodes.  */
>  static vec<ipa_reference_vars_info_t> ipa_reference_vars_vector;
>  
> +/* TODO: find a place where we should release the vector.  */
>  static vec<ipa_reference_optimization_summary_t> ipa_reference_opt_sum_vector;
>  
>  /* Return the ipa_reference_vars structure starting from the cgraph NODE.  */

Hmm, I guess this only makes sense for jitting, because the vector is used thorough
whole late complation (and should be turned into the summary template)

OK
Honza
> -- 
> 2.1.4
>
Martin Liška April 14, 2015, 2:14 p.m. UTC | #4 
On 04/14/2015 03:45 PM, Jan Hubicka wrote:
>>
>> 2015-04-13  Martin Liska  <mliska@suse.cz>
>>
>> 	* ipa-cp.c (ipcp_driver): Release prev_edge_clone.
>> 	* ipa-icf.c (sem_item_optimizer::subdivide_classes_by_sensitive_refs):
>> 	Release symbol_compare_collection.
>> 	* ipa-reference.c: Add TODO that a vector should be released.
>> ---
>>   gcc/ipa-cp.c        |  1 +
>>   gcc/ipa-icf.c       | 26 ++++++++++++++++++++------
>>   gcc/ipa-reference.c |  1 +
>>   3 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
>> index bfe4821..3824029 100644
>> --- a/gcc/ipa-cp.c
>> +++ b/gcc/ipa-cp.c
>> @@ -4493,6 +4493,7 @@ ipcp_driver (void)
>>     /* Free all IPCP structures.  */
>>     free_toporder_info (&topo);
>>     next_edge_clone.release ();
>> +  prev_edge_clone.release ();
>>     symtab->remove_edge_removal_hook (edge_removal_hook_holder);
>>     symtab->remove_edge_duplication_hook (edge_duplication_hook_holder);
>>     ipa_free_all_structures_after_ipa_cp ();
>
> I already fixed this one on mainline.

Ah, thanks.

>
>> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
>> index b902373..a72ac2e 100644
>> --- a/gcc/ipa-icf.c
>> +++ b/gcc/ipa-icf.c
>> @@ -2712,6 +2712,9 @@ sem_item_optimizer::subdivide_classes_by_equality (bool in_wpa)
>>   unsigned
>>   sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>>   {
>> +  typedef hash_map <symbol_compare_collection *, vec <sem_item *>,
>> +    symbol_compare_hashmap_traits> subdivide_hash_map;
>> +
>>     unsigned newly_created_classes = 0;
>>
>>     for (hash_table <congruence_class_group_hash>::iterator it = m_classes.begin ();
>> @@ -2726,8 +2729,7 @@ sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>>
>>   	  if (c->members.length() > 1)
>>   	    {
>> -	      hash_map <symbol_compare_collection *, vec <sem_item *>,
>> -		symbol_compare_hashmap_traits> split_map;
>> +	      subdivide_hash_map split_map;
>>
>>   	      for (unsigned j = 0; j < c->members.length (); j++)
>>   	        {
>> @@ -2735,10 +2737,15 @@ sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>>
>>   		  symbol_compare_collection *collection = new symbol_compare_collection (source_node->node);
>>
>> -		  vec <sem_item *> *slot = &split_map.get_or_insert (collection);
>> +		  bool existed;
>> +		  vec <sem_item *> *slot = &split_map.get_or_insert (collection,
>> +								     &existed);
>>   		  gcc_checking_assert (slot);
>>
>>   		  slot->safe_push (source_node);
>> +
>> +		  if (existed)
>> +		    delete collection;
>>   	        }
>>
>>   	       /* If the map contains more than one key, we have to split the map
>> @@ -2747,9 +2754,8 @@ sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>>   	        {
>>   		  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)
>> +		  for (subdivide_hash_map::iterator it2 = split_map.begin ();
>> +		       it2 != split_map.end (); ++it2)
>>   		    {
>>   		      congruence_class *new_cls;
>>   		      new_cls = new congruence_class (class_id++);
>> @@ -2772,6 +2778,14 @@ sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
>>   		        }
>>   		    }
>>   		}
>> +
>> +	      /* Release memory.  */
>> +	      for (subdivide_hash_map::iterator it2 = split_map.begin ();
>> +		   it2 != split_map.end (); ++it2)
>> +		{
>> +		  delete (*it2).first;
>> +		  (*it2).second.release ();
>> +		}
>>   	    }
>>   	  }
>>
>
> This is OK.

OK, I'm going to commit it to trunk.

>
>> diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
>> index 219a9b3..a420cb2 100644
>> --- a/gcc/ipa-reference.c
>> +++ b/gcc/ipa-reference.c
>> @@ -150,6 +150,7 @@ static struct cgraph_node_hook_list *node_removal_hook_holder;
>>      Indexed by UID of call graph nodes.  */
>>   static vec<ipa_reference_vars_info_t> ipa_reference_vars_vector;
>>
>> +/* TODO: find a place where we should release the vector.  */
>>   static vec<ipa_reference_optimization_summary_t> ipa_reference_opt_sum_vector;
>>
>>   /* Return the ipa_reference_vars structure starting from the cgraph NODE.  */
>
> Hmm, I guess this only makes sense for jitting, because the vector is used thorough
> whole late complation (and should be turned into the summary template)

Yes, I'll port it to new symbol_summary infrastructure.

Martin

>
> OK
> Honza
>> --
>> 2.1.4
>>
>
Jan Hubicka April 14, 2015, 3:20 p.m. UTC | #5 
> 
> OK, I'm going to commit it to trunk.

Thanks, and forgot to comment. I do not consider this critical for 5.1.
We may want to backport for 5.2.

Honza
diff mbox

Patch

From 5258b6a07d01c632976c3a5c3c5e47da3a9965e3 Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Mon, 13 Apr 2015 13:15:59 +0200
Subject: [PATCH] Fix IPA memory leaks.

gcc/ChangeLog:

2015-04-13  Martin Liska  <mliska@suse.cz>

	* ipa-cp.c (ipcp_driver): Release prev_edge_clone.
	* ipa-icf.c (sem_item_optimizer::subdivide_classes_by_sensitive_refs):
	Release symbol_compare_collection.
	* ipa-reference.c: Add TODO that a vector should be released.
---
 gcc/ipa-cp.c        |  1 +
 gcc/ipa-icf.c       | 26 ++++++++++++++++++++------
 gcc/ipa-reference.c |  1 +
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index bfe4821..3824029 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -4493,6 +4493,7 @@  ipcp_driver (void)
   /* Free all IPCP structures.  */
   free_toporder_info (&topo);
   next_edge_clone.release ();
+  prev_edge_clone.release ();
   symtab->remove_edge_removal_hook (edge_removal_hook_holder);
   symtab->remove_edge_duplication_hook (edge_duplication_hook_holder);
   ipa_free_all_structures_after_ipa_cp ();
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index b902373..a72ac2e 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -2712,6 +2712,9 @@  sem_item_optimizer::subdivide_classes_by_equality (bool in_wpa)
 unsigned
 sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
 {
+  typedef hash_map <symbol_compare_collection *, vec <sem_item *>,
+    symbol_compare_hashmap_traits> subdivide_hash_map;
+
   unsigned newly_created_classes = 0;
 
   for (hash_table <congruence_class_group_hash>::iterator it = m_classes.begin ();
@@ -2726,8 +2729,7 @@  sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
 
 	  if (c->members.length() > 1)
 	    {
-	      hash_map <symbol_compare_collection *, vec <sem_item *>,
-		symbol_compare_hashmap_traits> split_map;
+	      subdivide_hash_map split_map;
 
 	      for (unsigned j = 0; j < c->members.length (); j++)
 	        {
@@ -2735,10 +2737,15 @@  sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
 
 		  symbol_compare_collection *collection = new symbol_compare_collection (source_node->node);
 
-		  vec <sem_item *> *slot = &split_map.get_or_insert (collection);
+		  bool existed;
+		  vec <sem_item *> *slot = &split_map.get_or_insert (collection,
+								     &existed);
 		  gcc_checking_assert (slot);
 
 		  slot->safe_push (source_node);
+
+		  if (existed)
+		    delete collection;
 	        }
 
 	       /* If the map contains more than one key, we have to split the map
@@ -2747,9 +2754,8 @@  sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
 	        {
 		  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)
+		  for (subdivide_hash_map::iterator it2 = split_map.begin ();
+		       it2 != split_map.end (); ++it2)
 		    {
 		      congruence_class *new_cls;
 		      new_cls = new congruence_class (class_id++);
@@ -2772,6 +2778,14 @@  sem_item_optimizer::subdivide_classes_by_sensitive_refs ()
 		        }
 		    }
 		}
+
+	      /* Release memory.  */
+	      for (subdivide_hash_map::iterator it2 = split_map.begin ();
+		   it2 != split_map.end (); ++it2)
+		{
+		  delete (*it2).first;
+		  (*it2).second.release ();
+		}
 	    }
 	  }
 
diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 219a9b3..a420cb2 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -150,6 +150,7 @@  static struct cgraph_node_hook_list *node_removal_hook_holder;
    Indexed by UID of call graph nodes.  */
 static vec<ipa_reference_vars_info_t> ipa_reference_vars_vector;
 
+/* TODO: find a place where we should release the vector.  */
 static vec<ipa_reference_optimization_summary_t> ipa_reference_opt_sum_vector;
 
 /* Return the ipa_reference_vars structure starting from the cgraph NODE.  */
-- 
2.1.4