Re: [PATCH 5/9] ENABLE_CHECKING refactoring: pool allocators
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
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
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
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
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
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
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
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