[PATCH] Fix PR65538

[PATCH] Fix PR65538

[Martin Liška]

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

Hi.

In following patch, I've added missing delete call for all item summaries that are
allocated within a function_summary container in case the container does not use
GGC memory allocation.

Can boostrap on ppc64le and no regression is seen on x86_64-linux-pc.

Ready for trunk?
Thanks,
Martin

[-- Attachment #2: 0001-Fix-PR65538.patch --]
[-- Type: text/x-patch, Size: 1088 bytes --]

From c9912b88e8a381e6be7dc1e4be4f7b8859d72e2f Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Tue, 24 Mar 2015 13:58:50 +0100
Subject: [PATCH] Fix PR65538.

gcc/ChangeLog:

2015-03-24  Martin Liska  <mliska@suse.cz>

	PR tree-optimization/65538
	* symbol-summary.h (function_summary::~function_summary):
	Relese memory for allocated summaries in case non-GGC template
	instance.
---
 gcc/symbol-summary.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
index 8d7e42c..35615ba 100644
--- a/gcc/symbol-summary.h
+++ b/gcc/symbol-summary.h
@@ -81,6 +81,12 @@ public:
     m_symtab_insertion_hook = NULL;
     m_symtab_removal_hook = NULL;
     m_symtab_duplication_hook = NULL;
+
+    /* Release all summaries in case we use non-GGC memory.  */
+    typedef typename hash_map <int, T *, summary_hashmap_traits>::iterator map_iterator;
+    if (!m_ggc)
+      for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
+	delete (*it).second;
   }
 
   /* Traverses all summarys with a function F called with
-- 
2.1.4

Re: [PATCH] Fix PR65538

[Jan Hubicka]

> Hi.
> 
> In following patch, I've added missing delete call for all item summaries that are
> allocated within a function_summary container in case the container does not use
> GGC memory allocation.
> 
> Can boostrap on ppc64le and no regression is seen on x86_64-linux-pc.
> 
> Ready for trunk?
> Thanks,
> Martin

> >From c9912b88e8a381e6be7dc1e4be4f7b8859d72e2f Mon Sep 17 00:00:00 2001
> From: mliska <mliska@suse.cz>
> Date: Tue, 24 Mar 2015 13:58:50 +0100
> Subject: [PATCH] Fix PR65538.
> 
> gcc/ChangeLog:
> 
> 2015-03-24  Martin Liska  <mliska@suse.cz>
> 
> 	PR tree-optimization/65538
> 	* symbol-summary.h (function_summary::~function_summary):
> 	Relese memory for allocated summaries in case non-GGC template
> 	instance.
> ---
>  gcc/symbol-summary.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
> index 8d7e42c..35615ba 100644
> --- a/gcc/symbol-summary.h
> +++ b/gcc/symbol-summary.h
> @@ -81,6 +81,12 @@ public:
>      m_symtab_insertion_hook = NULL;
>      m_symtab_removal_hook = NULL;
>      m_symtab_duplication_hook = NULL;
> +
> +    /* Release all summaries in case we use non-GGC memory.  */
> +    typedef typename hash_map <int, T *, summary_hashmap_traits>::iterator map_iterator;
> +    if (!m_ggc)
> +      for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
> +	delete (*it).second;

I think you sould also do the walk with GGC memory and call ggc_free.
During WPA we almost never call ggc_collect so it is better to keep things explicitly freed.
OK with that change.

Honza

Re: [PATCH] Fix PR65538

[Martin Liška]

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

On 03/24/2015 06:38 PM, Jan Hubicka wrote:
>> Hi.
>>
>> In following patch, I've added missing delete call for all item summaries that are
>> allocated within a function_summary container in case the container does not use
>> GGC memory allocation.
>>
>> Can boostrap on ppc64le and no regression is seen on x86_64-linux-pc.
>>
>> Ready for trunk?
>> Thanks,
>> Martin
> 
>> >From c9912b88e8a381e6be7dc1e4be4f7b8859d72e2f Mon Sep 17 00:00:00 2001
>> From: mliska <mliska@suse.cz>
>> Date: Tue, 24 Mar 2015 13:58:50 +0100
>> Subject: [PATCH] Fix PR65538.
>>
>> gcc/ChangeLog:
>>
>> 2015-03-24  Martin Liska  <mliska@suse.cz>
>>
>> 	PR tree-optimization/65538
>> 	* symbol-summary.h (function_summary::~function_summary):
>> 	Relese memory for allocated summaries in case non-GGC template
>> 	instance.
>> ---
>>  gcc/symbol-summary.h | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
>> index 8d7e42c..35615ba 100644
>> --- a/gcc/symbol-summary.h
>> +++ b/gcc/symbol-summary.h
>> @@ -81,6 +81,12 @@ public:
>>      m_symtab_insertion_hook = NULL;
>>      m_symtab_removal_hook = NULL;
>>      m_symtab_duplication_hook = NULL;
>> +
>> +    /* Release all summaries in case we use non-GGC memory.  */
>> +    typedef typename hash_map <int, T *, summary_hashmap_traits>::iterator map_iterator;
>> +    if (!m_ggc)
>> +      for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
>> +	delete (*it).second;
> 
> I think you sould also do the walk with GGC memory and call ggc_free.
> During WPA we almost never call ggc_collect so it is better to keep things explicitly freed.
> OK with that change.
> 
> Honza
> 

There's updated version of patch, I've been testing. I'm going to install the patch
after it finishes.

Thanks,
Martin

[-- Attachment #2: 0001-Fix-PR65538.patch --]
[-- Type: text/x-patch, Size: 1447 bytes --]

From 8ae68cd2c69287c26543b22fa7afe2ff5cdcda8c Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Tue, 24 Mar 2015 13:58:50 +0100
Subject: [PATCH] Fix PR65538.

gcc/ChangeLog:

2015-03-24  Martin Liska  <mliska@suse.cz>

	PR tree-optimization/65538
	* symbol-summary.h (function_summary::~function_summary):
	Relese memory for allocated summaries in case non-GGC template
	instance.
---
 gcc/symbol-summary.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
index 8d7e42c..9a87891 100644
--- a/gcc/symbol-summary.h
+++ b/gcc/symbol-summary.h
@@ -81,6 +81,12 @@ public:
     m_symtab_insertion_hook = NULL;
     m_symtab_removal_hook = NULL;
     m_symtab_duplication_hook = NULL;
+
+    /* Release all summaries in case we use non-GGC memory.  */
+    typedef typename hash_map <int, T *, summary_hashmap_traits>::iterator map_iterator;
+    if (!m_ggc)
+      for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
+	release ((*it).second);
   }
 
   /* Traverses all summarys with a function F called with
@@ -106,6 +112,15 @@ public:
     return m_ggc ? new (ggc_alloc <T> ()) T() : new T () ;
   }
 
+  /* Release an item that is stored within map.  */
+  void release (T *item)
+  {
+    if (m_ggc)
+      ggc_free (item);
+    else
+      delete item;
+  }
+
   /* Getter for summary callgraph node pointer.  */
   T* get (cgraph_node *node)
   {
-- 
2.1.4

Re: [PATCH] Fix PR65538

[Jakub Jelinek]

On Tue, Mar 24, 2015 at 10:54:25PM +0100, Martin Liška wrote:
> --- a/gcc/symbol-summary.h
> +++ b/gcc/symbol-summary.h
> @@ -81,6 +81,12 @@ public:
>      m_symtab_insertion_hook = NULL;
>      m_symtab_removal_hook = NULL;
>      m_symtab_duplication_hook = NULL;
> +
> +    /* Release all summaries in case we use non-GGC memory.  */
> +    typedef typename hash_map <int, T *, summary_hashmap_traits>::iterator map_iterator;
> +    if (!m_ggc)
> +      for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
> +	release ((*it).second);

You haven't removed the now unnecessary if (!m_ggc) guard.

> @@ -106,6 +112,15 @@ public:
>      return m_ggc ? new (ggc_alloc <T> ()) T() : new T () ;
>    }
>  
> +  /* Release an item that is stored within map.  */
> +  void release (T *item)
> +  {
> +    if (m_ggc)
> +      ggc_free (item);

Perhaps run also the item's destructor first?  I know that
inline_summary doesn't have a user destructor, so it will expand to nothing,
so it would be just for completeness.

> +    else
> +      delete item;
> +  }
> +

	Jakub

Re: [PATCH] Fix PR65538

[Jan Hubicka]

> On Tue, Mar 24, 2015 at 10:54:25PM +0100, Martin Liška wrote:
> > --- a/gcc/symbol-summary.h
> > +++ b/gcc/symbol-summary.h
> > @@ -81,6 +81,12 @@ public:
> >      m_symtab_insertion_hook = NULL;
> >      m_symtab_removal_hook = NULL;
> >      m_symtab_duplication_hook = NULL;
> > +
> > +    /* Release all summaries in case we use non-GGC memory.  */
> > +    typedef typename hash_map <int, T *, summary_hashmap_traits>::iterator map_iterator;
> > +    if (!m_ggc)
> > +      for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
> > +	release ((*it).second);
> 
> You haven't removed the now unnecessary if (!m_ggc) guard.
> 
> > @@ -106,6 +112,15 @@ public:
> >      return m_ggc ? new (ggc_alloc <T> ()) T() : new T () ;
> >    }
> >  
> > +  /* Release an item that is stored within map.  */
> > +  void release (T *item)
> > +  {
> > +    if (m_ggc)
> > +      ggc_free (item);
> 
> Perhaps run also the item's destructor first?  I know that
> inline_summary doesn't have a user destructor, so it will expand to nothing,
> so it would be just for completeness.

Yep, calling destructors is a good idea.  OK with that change
and fix Jakub pointed out.

Honza
> 
> > +    else
> > +      delete item;
> > +  }
> > +
> 
> 	Jakub

Re: [PATCH] Fix PR65538

[Martin Liška]

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

On 03/25/2015 12:37 AM, Jan Hubicka wrote:
>> On Tue, Mar 24, 2015 at 10:54:25PM +0100, Martin Liška wrote:
>>> --- a/gcc/symbol-summary.h
>>> +++ b/gcc/symbol-summary.h
>>> @@ -81,6 +81,12 @@ public:
>>>       m_symtab_insertion_hook = NULL;
>>>       m_symtab_removal_hook = NULL;
>>>       m_symtab_duplication_hook = NULL;
>>> +
>>> +    /* Release all summaries in case we use non-GGC memory.  */
>>> +    typedef typename hash_map <int, T *, summary_hashmap_traits>::iterator map_iterator;
>>> +    if (!m_ggc)
>>> +      for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
>>> +	release ((*it).second);
>>
>> You haven't removed the now unnecessary if (!m_ggc) guard.
>>
>>> @@ -106,6 +112,15 @@ public:
>>>       return m_ggc ? new (ggc_alloc <T> ()) T() : new T () ;
>>>     }
>>>
>>> +  /* Release an item that is stored within map.  */
>>> +  void release (T *item)
>>> +  {
>>> +    if (m_ggc)
>>> +      ggc_free (item);
>>
>> Perhaps run also the item's destructor first?  I know that
>> inline_summary doesn't have a user destructor, so it will expand to nothing,
>> so it would be just for completeness.
>
> Yep, calling destructors is a good idea.  OK with that change
> and fix Jakub pointed out.
>
> Honza
>>
>>> +    else
>>> +      delete item;
>>> +  }
>>> +
>>
>> 	Jakub

Ok, changes are applied in the final patch I'm going to install.

Thanks,
Martin

[-- Attachment #2: 0001-Fix-PR65538.patch --]
[-- Type: text/x-patch, Size: 1442 bytes --]

From 6eae938e34e36c461ebec1570ff0f3d2f5e1b8cf Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Tue, 24 Mar 2015 13:58:50 +0100
Subject: [PATCH] Fix PR65538.

gcc/ChangeLog:

2015-03-24  Martin Liska  <mliska@suse.cz>

	PR tree-optimization/65538
	* symbol-summary.h (function_summary::~function_summary):
	Relese memory for allocated summaries.
	(function_summary::release): New function.
---
 gcc/symbol-summary.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
index 8d7e42c..0448310 100644
--- a/gcc/symbol-summary.h
+++ b/gcc/symbol-summary.h
@@ -81,6 +81,11 @@ public:
     m_symtab_insertion_hook = NULL;
     m_symtab_removal_hook = NULL;
     m_symtab_duplication_hook = NULL;
+
+    /* Release all summaries.  */
+    typedef typename hash_map <int, T *, summary_hashmap_traits>::iterator map_iterator;
+    for (map_iterator it = m_map.begin (); it != m_map.end (); ++it)
+      release ((*it).second);
   }
 
   /* Traverses all summarys with a function F called with
@@ -106,6 +111,18 @@ public:
     return m_ggc ? new (ggc_alloc <T> ()) T() : new T () ;
   }
 
+  /* Release an item that is stored within map.  */
+  void release (T *item)
+  {
+    if (m_ggc)
+      {
+	item->~T ();
+	ggc_free (item);
+      }
+    else
+      delete item;
+  }
+
   /* Getter for summary callgraph node pointer.  */
   T* get (cgraph_node *node)
   {
-- 
2.1.4