Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators

[Mikhail Maltsev]

On 10/26/2015 12:47 PM, Richard Biener wrote:
> I committed the attached to fix build with the valgrind annotations active.
> 
> Richard.
> 
Doh! Sorry for breakage.

-- 
Regards,
Mikhail Maltsev

Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators

[Richard Biener]

On Mon, Oct 26, 2015 at 2:18 AM, Mikhail Maltsev  wrote:
> On 10/21/2015 01:57 PM, Richard Biener wrote:
>> Ugh (stupid templates).
>>
>> @@ -387,10 +389,10 @@ base_pool_allocator ::allocate ()
>>block = m_virgin_free_list;
>>header = (allocation_pool_list*) allocation_object::get_data (block);
>>header->next = NULL;
>> -#ifdef ENABLE_CHECKING
>> +
>>/* Mark the element to be free.  */
>> -  ((allocation_object*) block)->id = 0;
>> -#endif
>> +  if (flag_checking)
>> +   ((allocation_object*) block)->id = 0;
>>
>> just set id to zero unconditionally.  That'll be faster than checking
>> flag_checking.
>
> I fixed this and other issues, and committed the attached patch.

I committed the attached to fix build with the valgrind annotations active.

Richard.

2015-10-26  Richard Biener  

* alloc-pool.h (base_pool_allocator): Use placement new.
(base_pool_allocator::remove): Likewise.  Compute size outside of
flag_checking.


> --
> Regards,
> Mikhail Maltsev


p
Description: Binary data

Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators

[Mikhail Maltsev]

On 10/21/2015 01:57 PM, Richard Biener wrote:
> Ugh (stupid templates).
> 
> @@ -387,10 +389,10 @@ base_pool_allocator ::allocate ()
>block = m_virgin_free_list;
>header = (allocation_pool_list*) allocation_object::get_data (block);
>header->next = NULL;
> -#ifdef ENABLE_CHECKING
> +
>/* Mark the element to be free.  */
> -  ((allocation_object*) block)->id = 0;
> -#endif
> +  if (flag_checking)
> +   ((allocation_object*) block)->id = 0;
> 
> just set id to zero unconditionally.  That'll be faster than checking
> flag_checking.

I fixed this and other issues, and committed the attached patch.

-- 
Regards,
Mikhail Maltsev
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 81d0e1c..d8a22c3 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2015-10-26  Mikhail Maltsev  
+
+	* alloc-pool.h (base_pool_allocator::initialize, ::allocate): Remove
+	conditional compilation.
+	(base_pool_allocator::remove): Use flag_checking.
+
 2015-10-25  John David Anglin  
 
 	* config/pa/som.h (EH_FRAME_THROUGH_COLLECT2): Define.
diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
index 70105ba..404b558 100644
--- a/gcc/alloc-pool.h
+++ b/gcc/alloc-pool.h
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
 #define ALLOC_POOL_H
 
 #include "memory-block.h"
+#include "options.h"	// for flag_checking
 
 extern void dump_alloc_pool_statistics (void);
 
@@ -275,7 +276,6 @@ base_pool_allocator ::initialize ()
   m_elts_per_block = (TBlockAllocator::block_size - header_size) / size;
   gcc_checking_assert (m_elts_per_block != 0);
 
-#ifdef ENABLE_CHECKING
   /* Increase the last used ID and use it for this pool.
  ID == 0 is used for free elements of pool so skip it.  */
   last_id++;
@@ -283,7 +283,6 @@ base_pool_allocator ::initialize ()
 last_id++;
 
   m_id = last_id;
-#endif
 }
 
 /* Free all memory allocated for the given memory pool.  */
@@ -387,10 +386,9 @@ base_pool_allocator ::allocate ()
   block = m_virgin_free_list;
   header = (allocation_pool_list*) allocation_object::get_data (block);
   header->next = NULL;
-#ifdef ENABLE_CHECKING
+
   /* Mark the element to be free.  */
   ((allocation_object*) block)->id = 0;
-#endif
   VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS (header,size));
   m_returned_free_list = header;
   m_virgin_free_list += m_elt_size;
@@ -404,10 +402,8 @@ base_pool_allocator ::allocate ()
   m_returned_free_list = header->next;
   m_elts_free--;
 
-#ifdef ENABLE_CHECKING
   /* Set the ID for element.  */
   allocation_object::get_instance (header)->id = m_id;
-#endif
   VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
 
   return (void *)(header);
@@ -418,26 +414,23 @@ template 
 inline void
 base_pool_allocator ::remove (void *object)
 {
-  gcc_checking_assert (m_initialized);
-
-  allocation_pool_list *header;
-  int size ATTRIBUTE_UNUSED;
-  size = m_elt_size - offsetof (allocation_object, u.data);
-
-#ifdef ENABLE_CHECKING
-  gcc_assert (object
+  if (flag_checking)
+{
+  gcc_assert (m_initialized);
+  gcc_assert (object
 	  /* Check if we free more than we allocated, which is Bad (TM).  */
 	  && m_elts_free < m_elts_allocated
 	  /* Check whether the PTR was allocated from POOL.  */
 	  && m_id == allocation_object::get_instance (object)->id);
 
-  memset (object, 0xaf, size);
+  int size = m_elt_size - offsetof (allocation_object, u.data);
+  memset (object, 0xaf, size);
+}
 
   /* Mark the element to be free.  */
   allocation_object::get_instance (object)->id = 0;
-#endif
 
-  header = (allocation_pool_list*) object;
+  allocation_pool_list *header = (allocation_pool_list*) object;
   header->next = m_returned_free_list;
   m_returned_free_list = header;
   VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS (object, size));

Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators

[Richard Biener]

On Mon, Oct 19, 2015 at 2:09 AM, Mikhail Maltsev  wrote:
> On 10/06/2015 03:45 PM, Richard Biener wrote:
>> On Tue, Oct 6, 2015 at 2:41 PM, Bernd Schmidt  wrote:
>>> On 10/06/2015 01:32 AM, Mikhail Maltsev wrote:

 gcc/ChangeLog:

 2015-10-05  Mikhail Maltsev  

 * alloc-pool.h (base_pool_allocator::initialize, ::allocate,
 ::remove): Adjust to use CHECKING_P.
>>>
>>>
>>> Why CHECKING_P for these and not flag_checking as elsewhere?
>>
>> Probably because they are in inline functions and thus possibly would
>> affect optimization.  Not sure why they are inline functions in the
>> first place...  I'd agree to using flag_checking here.
>>
>> Richard.
>>
>>>
>>> Bernd
>
> Adjusted. Note: I had to include 'options.h' into 'alloc-pool.h' in order to 
> use
> flag_checking.

Ugh (stupid templates).

@@ -387,10 +389,10 @@ base_pool_allocator ::allocate ()
   block = m_virgin_free_list;
   header = (allocation_pool_list*) allocation_object::get_data (block);
   header->next = NULL;
-#ifdef ENABLE_CHECKING
+
   /* Mark the element to be free.  */
-  ((allocation_object*) block)->id = 0;
-#endif
+  if (flag_checking)
+   ((allocation_object*) block)->id = 0;

just set id to zero unconditionally.  That'll be faster than checking
flag_checking.

-#ifdef ENABLE_CHECKING
   /* Set the ID for element.  */
-  allocation_object::get_instance (header)->id = m_id;
-#endif
+  if (flag_checking)
+allocation_object::get_instance (header)->id = m_id;

likewise.

Given that, the pool initialization itself can do with unconditonally
computing the id as well, thus

-#ifdef ENABLE_CHECKING
-  /* Increase the last used ID and use it for this pool.
- ID == 0 is used for free elements of pool so skip it.  */
-  last_id++;
-  if (last_id == 0)
-last_id++;
+  if (flag_checking)
+{
+  /* Increase the last used ID and use it for this pool.
+ID == 0 is used for free elements of pool so skip it.  */
+  last_id++;
+  if (last_id == 0)
+   last_id++;

-  m_id = last_id;
-#endif
+  m_id = last_id;
+}

make that unconditionally enabled as well.

-#ifdef ENABLE_CHECKING
-  gcc_assert (object
+  gcc_checking_assert (object
  /* Check if we free more than we allocated, which is Bad (TM).  */
  && m_elts_free < m_elts_allocated
  /* Check whether the PTR was allocated from POOL.  */
  && m_id == allocation_object::get_instance (object)->id);

-  memset (object, 0xaf, size);
-
-  /* Mark the element to be free.  */
-  allocation_object::get_instance (object)->id = 0;
-#endif
+  if (flag_checking)
+{
+  memset (object, 0xaf, size);
+  /* Mark the element to be free.  */
+  allocation_object::get_instance (object)->id = 0;
+}


guard the assert with flag_checking and do the id set unconditionally
(for consistency).

Ok with those changes.

Thanks,
Richard.


> --
> Regards,
> Mikhail Maltsev
>
> 2015-10-19  Mikhail Maltsev  
>
> * alloc-pool.h (base_pool_allocator ::initialize, ::allocate,
> ::remove): Adjust to use flag_checking.

Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators

[Mikhail Maltsev]

On 10/06/2015 03:45 PM, Richard Biener wrote:
> On Tue, Oct 6, 2015 at 2:41 PM, Bernd Schmidt  wrote:
>> On 10/06/2015 01:32 AM, Mikhail Maltsev wrote:
>>>
>>> gcc/ChangeLog:
>>>
>>> 2015-10-05  Mikhail Maltsev  
>>>
>>> * alloc-pool.h (base_pool_allocator::initialize, ::allocate,
>>> ::remove): Adjust to use CHECKING_P.
>>
>>
>> Why CHECKING_P for these and not flag_checking as elsewhere?
> 
> Probably because they are in inline functions and thus possibly would
> affect optimization.  Not sure why they are inline functions in the
> first place...  I'd agree to using flag_checking here.
> 
> Richard.
> 
>>
>> Bernd

Adjusted. Note: I had to include 'options.h' into 'alloc-pool.h' in order to use
flag_checking.

-- 
Regards,
Mikhail Maltsev

2015-10-19  Mikhail Maltsev  

* alloc-pool.h (base_pool_allocator ::initialize, ::allocate,
::remove): Adjust to use flag_checking.
diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
index 70105ba..a15c25e 100644
--- a/gcc/alloc-pool.h
+++ b/gcc/alloc-pool.h
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
 #define ALLOC_POOL_H
 
 #include "memory-block.h"
+#include "options.h"	// for flag_checking
 
 extern void dump_alloc_pool_statistics (void);
 
@@ -275,15 +276,16 @@ base_pool_allocator ::initialize ()
   m_elts_per_block = (TBlockAllocator::block_size - header_size) / size;
   gcc_checking_assert (m_elts_per_block != 0);
 
-#ifdef ENABLE_CHECKING
-  /* Increase the last used ID and use it for this pool.
- ID == 0 is used for free elements of pool so skip it.  */
-  last_id++;
-  if (last_id == 0)
-last_id++;
+  if (flag_checking)
+{
+  /* Increase the last used ID and use it for this pool.
+	 ID == 0 is used for free elements of pool so skip it.  */
+  last_id++;
+  if (last_id == 0)
+	last_id++;
 
-  m_id = last_id;
-#endif
+  m_id = last_id;
+}
 }
 
 /* Free all memory allocated for the given memory pool.  */
@@ -387,10 +389,10 @@ base_pool_allocator ::allocate ()
   block = m_virgin_free_list;
   header = (allocation_pool_list*) allocation_object::get_data (block);
   header->next = NULL;
-#ifdef ENABLE_CHECKING
+
   /* Mark the element to be free.  */
-  ((allocation_object*) block)->id = 0;
-#endif
+  if (flag_checking)
+	((allocation_object*) block)->id = 0;
   VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS (header,size));
   m_returned_free_list = header;
   m_virgin_free_list += m_elt_size;
@@ -404,10 +406,9 @@ base_pool_allocator ::allocate ()
   m_returned_free_list = header->next;
   m_elts_free--;
 
-#ifdef ENABLE_CHECKING
   /* Set the ID for element.  */
-  allocation_object::get_instance (header)->id = m_id;
-#endif
+  if (flag_checking)
+allocation_object::get_instance (header)->id = m_id;
   VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
 
   return (void *)(header);
@@ -424,18 +425,18 @@ base_pool_allocator ::remove (void *object)
   int size ATTRIBUTE_UNUSED;
   size = m_elt_size - offsetof (allocation_object, u.data);
 
-#ifdef ENABLE_CHECKING
-  gcc_assert (object
+  gcc_checking_assert (object
 	  /* Check if we free more than we allocated, which is Bad (TM).  */
 	  && m_elts_free < m_elts_allocated
 	  /* Check whether the PTR was allocated from POOL.  */
 	  && m_id == allocation_object::get_instance (object)->id);
 
-  memset (object, 0xaf, size);
-
-  /* Mark the element to be free.  */
-  allocation_object::get_instance (object)->id = 0;
-#endif
+  if (flag_checking)
+{
+  memset (object, 0xaf, size);
+  /* Mark the element to be free.  */
+  allocation_object::get_instance (object)->id = 0;
+}
 
   header = (allocation_pool_list*) object;
   header->next = m_returned_free_list;

Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators

[Richard Biener]

On Tue, Oct 6, 2015 at 2:41 PM, Bernd Schmidt  wrote:
> On 10/06/2015 01:32 AM, Mikhail Maltsev wrote:
>>
>> gcc/ChangeLog:
>>
>> 2015-10-05  Mikhail Maltsev  
>>
>> * alloc-pool.h (base_pool_allocator::initialize, ::allocate,
>> ::remove): Adjust to use CHECKING_P.
>
>
> Why CHECKING_P for these and not flag_checking as elsewhere?

Probably because they are in inline functions and thus possibly would
affect optimization.  Not sure why they are inline functions in the
first place...  I'd agree to using flag_checking here.

Richard.

>
> Bernd

Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators

[Bernd Schmidt]

On 10/06/2015 01:32 AM, Mikhail Maltsev wrote:

gcc/ChangeLog:

2015-10-05  Mikhail Maltsev  

* alloc-pool.h (base_pool_allocator::initialize, ::allocate,
::remove): Adjust to use CHECKING_P.


Why CHECKING_P for these and not flag_checking as elsewhere?


Bernd

Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators

[Mikhail Maltsev]

gcc/ChangeLog:

2015-10-05  Mikhail Maltsev  

* alloc-pool.h (base_pool_allocator::initialize, ::allocate,
::remove): Adjust to use CHECKING_P.

>From ed727b2279dd36e2fbf1ab6956270cbd487b1a01 Mon Sep 17 00:00:00 2001
From: Mikhail Maltsev 
Date: Sun, 4 Oct 2015 22:50:40 +0300
Subject: [PATCH 5/9] Allocators

---
 gcc/alloc-pool.h | 45 ++---
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
index 70105ba..8477ee4 100644
--- a/gcc/alloc-pool.h
+++ b/gcc/alloc-pool.h
@@ -275,15 +275,16 @@ base_pool_allocator ::initialize ()
   m_elts_per_block = (TBlockAllocator::block_size - header_size) / size;
   gcc_checking_assert (m_elts_per_block != 0);
 
-#ifdef ENABLE_CHECKING
-  /* Increase the last used ID and use it for this pool.
- ID == 0 is used for free elements of pool so skip it.  */
-  last_id++;
-  if (last_id == 0)
-last_id++;
-
-  m_id = last_id;
-#endif
+  if (CHECKING_P)
+{
+  /* Increase the last used ID and use it for this pool.
+	 ID == 0 is used for free elements of pool so skip it.  */
+  last_id++;
+  if (last_id == 0)
+	last_id++;
+
+  m_id = last_id;
+}
 }
 
 /* Free all memory allocated for the given memory pool.  */
@@ -387,10 +388,9 @@ base_pool_allocator ::allocate ()
   block = m_virgin_free_list;
   header = (allocation_pool_list*) allocation_object::get_data (block);
   header->next = NULL;
-#ifdef ENABLE_CHECKING
-  /* Mark the element to be free.  */
-  ((allocation_object*) block)->id = 0;
-#endif
+  if (CHECKING_P)
+	/* Mark the element to be free.  */
+	((allocation_object*) block)->id = 0;
   VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS (header,size));
   m_returned_free_list = header;
   m_virgin_free_list += m_elt_size;
@@ -404,10 +404,9 @@ base_pool_allocator ::allocate ()
   m_returned_free_list = header->next;
   m_elts_free--;
 
-#ifdef ENABLE_CHECKING
   /* Set the ID for element.  */
-  allocation_object::get_instance (header)->id = m_id;
-#endif
+  if (CHECKING_P)
+allocation_object::get_instance (header)->id = m_id;
   VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
 
   return (void *)(header);
@@ -424,18 +423,18 @@ base_pool_allocator ::remove (void *object)
   int size ATTRIBUTE_UNUSED;
   size = m_elt_size - offsetof (allocation_object, u.data);
 
-#ifdef ENABLE_CHECKING
-  gcc_assert (object
+  gcc_checking_assert (object
 	  /* Check if we free more than we allocated, which is Bad (TM).  */
 	  && m_elts_free < m_elts_allocated
 	  /* Check whether the PTR was allocated from POOL.  */
 	  && m_id == allocation_object::get_instance (object)->id);
 
-  memset (object, 0xaf, size);
-
-  /* Mark the element to be free.  */
-  allocation_object::get_instance (object)->id = 0;
-#endif
+  if (CHECKING_P)
+{
+  memset (object, 0xaf, size);
+  /* Mark the element to be free.  */
+  allocation_object::get_instance (object)->id = 0;
+}
 
   header = (allocation_pool_list*) object;
   header->next = m_returned_free_list;
-- 
2.1.4