Re: [PATCH 01/35] Introduce new type-based pool allocator.
In file included from ../../gcc/stmt.c:78:0: ../../gcc/alloc-pool.h: In function 'void expand_sjlj_dispatch_table(rtx, vect\ ree_node*)': ../../gcc/alloc-pool.h:303:4: error: 'case_node_pool.pool_allocatorcase_node:\ :m_block_size' may be used uninitialized in this function [-Werror=maybe-uninit\ ialized] block = XNEWVEC (char, m_block_size); ^ ../../gcc/stmt.c:1339:33: note: 'case_node_pool.pool_allocatorcase_node::m_bl\ ock_size' was declared here pool_allocatorcase_node case_node_pool (struct sjlj_case pool, ^ Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
Re: [PATCH 01/35] Introduce new type-based pool allocator.
On Tue, Jun 2, 2015 at 3:52 PM, Martin Liška mli...@suse.cz wrote: On 06/02/2015 11:48 AM, Andreas Schwab wrote: In file included from ../../gcc/stmt.c:78:0: ../../gcc/alloc-pool.h: In function 'void expand_sjlj_dispatch_table(rtx, vect\ ree_node*)': ../../gcc/alloc-pool.h:303:4: error: 'case_node_pool.pool_allocatorcase_node:\ :m_block_size' may be used uninitialized in this function [-Werror=maybe-uninit\ ialized] block = XNEWVEC (char, m_block_size); ^ ../../gcc/stmt.c:1339:33: note: 'case_node_pool.pool_allocatorcase_node::m_bl\ ock_size' was declared here pool_allocatorcase_node case_node_pool (struct sjlj_case pool, ^ Andreas. Hi. This patch for the issue which has been tested on x86_64-unknown-linux-pc and can bootstrap. Ready for trunk? Ok. Richard. Thanks, Martin
Re: [PATCH 01/35] Introduce new type-based pool allocator.
On 06/02/2015 11:48 AM, Andreas Schwab wrote:
In file included from ../../gcc/stmt.c:78:0:
../../gcc/alloc-pool.h: In function 'void expand_sjlj_dispatch_table(rtx,
vect\
ree_node*)':
../../gcc/alloc-pool.h:303:4: error:
'case_node_pool.pool_allocatorcase_node:\
:m_block_size' may be used uninitialized in this function
[-Werror=maybe-uninit\
ialized]
block = XNEWVEC (char, m_block_size);
^
../../gcc/stmt.c:1339:33: note:
'case_node_pool.pool_allocatorcase_node::m_bl\
ock_size' was declared here
pool_allocatorcase_node case_node_pool (struct sjlj_case pool,
^
Andreas.
Hi.
This patch for the issue which has been tested on x86_64-unknown-linux-pc and
can bootstrap.
Ready for trunk?
Thanks,
Martin
From 57355c1e271accc3e35dd5df9d5393ee783d765b Mon Sep 17 00:00:00 2001
From: mliska mli...@suse.cz
Date: Tue, 2 Jun 2015 13:26:05 +0200
Subject: [PATCH] Pool allocator fallout: fix uninialized class members.
gcc/ChangeLog:
2015-06-02 Martin Liska mli...@suse.cz
* alloc-pool.h (pool_allocator::pool_allocator): Set implicit
values to avoid -Wmaybe-uninitialized errors.
---
gcc/alloc-pool.h | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
index 96a1342..ddb2a91 100644
--- a/gcc/alloc-pool.h
+++ b/gcc/alloc-pool.h
@@ -159,11 +159,11 @@ template typename T
inline
pool_allocatorT::pool_allocator (const char *name, size_t num,
size_t extra_size, bool ignore_type_size):
- m_name (name), m_elts_per_block (num), m_returned_free_list (NULL),
+ m_name (name), m_id (0), m_elts_per_block (num), m_returned_free_list (NULL),
m_virgin_free_list (NULL), m_virgin_elts_remaining (0), m_elts_allocated (0),
m_elts_free (0), m_blocks_allocated (0), m_block_list (NULL),
- m_ignore_type_size (ignore_type_size), m_extra_size (extra_size),
- m_initialized (false) {}
+ m_block_size (0), m_ignore_type_size (ignore_type_size),
+ m_extra_size (extra_size), m_initialized (false) {}
/* Initialize a pool allocator. */
@@ -215,7 +215,6 @@ pool_allocatorT::initialize ()
m_id = last_id;
#endif
-
}
/* Free all memory allocated for the given memory pool. */
--
2.1.4
Re: [PATCH 01/35] Introduce new type-based pool allocator.
On 05/28/2015 07:15 PM, Jeff Law wrote:
On 05/28/2015 06:49 AM, Martin Liška wrote:
.
This mechanism has been just adapted. I find it quite useful as we have
examples in source code where we
allocate same struct/class types from a various pool. For debugging
purpose, it helps to identify if
release operation is called for a correct pool.
I saw that you were following existing practice for the pools in the removal
patch. I still don't like it as it makes mixing and matching objects harder
when debugging gcc and if the structure is exposed for plugins, then we've got
an unnecessary ABI plugin breakage.
I certainly understand how it's useful -- I'm not questioning that. I'm
questioning changing the size of structures on ENABLE_CHECKING.
My first inclination would be to include all that stuff unconditionally. If
that's too much overhead, then perhaps include the structure member, but not
bother with any of the bookkeeping except for ENABLE_CHECKING.
Hi.
Updated version of patch removes ENABLE_CHECKING in the struct definition.
News in the patchset I'm going to re-send:
+ Changelog entries are fixed for spaces
+ Each patch passes ./contrib/check_GNU_style.sh script
+ pool_allocator::pool_allocator is a trivial constructor and first allocation
launches initialization
+ some patches are squashed as were mentioned multiple time (ira-color.c,
ira-build.c)
The patch set survives x86_64-linux-pc boostrap, I'm going to re-run regression
tests.
Thanks,
Martin
Anyway, I would like to commit all these patches at once (one by one).
Thus, I'm going to wait for approval for the whole series before I'll
commit the set.
Quite reasonable -- I was mostly trying to make sure I understood the testing
situation.
I think at this point the whole series is approved, so you can move forward.
jeff
From b551d2349b342776213f518de0835ab54fa9fa03 Mon Sep 17 00:00:00 2001
From: mliska mli...@suse.cz
Date: Wed, 27 May 2015 15:56:44 +0200
Subject: [PATCH 01/32] Introduce new type-based pool allocator.
Hello.
Following patch set attempts to replace old-style pool allocator
to a type-based one. Moreover, as we utilize classes and structs that are used
just by a pool allocator, these types have overwritten ctors and dtors.
Thus, using the allocator is much easier and we shouldn't cast types
back and forth. Another beneficat can be achieved in future, as we will
be able to call a class constructors to correctly register a location,
where a memory is allocated (-fgather-detailed-mem-stats).
Patch can boostrap on x86_64-linux-gnu and ppc64-linux-gnu and
survives regression tests on x86_64-linux-gnu.
Ready for trunk?
Thanks,
Martin
gcc/ChangeLog:
2015-04-30 Martin Liska mli...@suse.cz
* alloc-pool.c (struct alloc_pool_descriptor): Move definition
to header file.
* alloc-pool.h (pool_allocator::pool_allocator): New function.
(pool_allocator::release): Likewise.
(inline pool_allocator::release_if_empty): Likewise.
(inline pool_allocator::~pool_allocator): Likewise.
(pool_allocator::allocate): Likewise.
(pool_allocator::remove): Likewise.
---
gcc/alloc-pool.c | 33 +
gcc/alloc-pool.h | 380 ++-
2 files changed, 383 insertions(+), 30 deletions(-)
diff --git a/gcc/alloc-pool.c b/gcc/alloc-pool.c
index 81909d8..0bea7a6 100644
--- a/gcc/alloc-pool.c
+++ b/gcc/alloc-pool.c
@@ -25,6 +25,8 @@ along with GCC; see the file COPYING3. If not see
#include hash-table.h
#include hash-map.h
+ALLOC_POOL_ID_TYPE last_id;
+
#define align_eight(x) (((x+7) 3) 3)
/* The internal allocation object. */
@@ -58,36 +60,10 @@ typedef struct allocation_object_def
#define USER_PTR_FROM_ALLOCATION_OBJECT_PTR(X)\
((void *) (((allocation_object *) (X))-u.data))
-#ifdef ENABLE_CHECKING
-/* Last used ID. */
-static ALLOC_POOL_ID_TYPE last_id;
-#endif
-
-/* Store information about each particular alloc_pool. Note that this
- will underestimate the amount the amount of storage used by a small amount:
- 1) The overhead in a pool is not accounted for.
- 2) The unallocated elements in a block are not accounted for. Note
- that this can at worst case be one element smaller that the block
- size for that pool. */
-struct alloc_pool_descriptor
-{
- /* Number of pools allocated. */
- unsigned long created;
- /* Gross allocated storage. */
- unsigned long allocated;
- /* Amount of currently active storage. */
- unsigned long current;
- /* Peak amount of storage used. */
- unsigned long peak;
- /* Size of element in the pool. */
- int elt_size;
-};
-
/* Hashtable mapping alloc_pool names to descriptors. */
-static hash_mapconst char *, alloc_pool_descriptor *alloc_pool_hash;
+hash_mapconst char *, alloc_pool_descriptor *alloc_pool_hash;
-/* For given name, return descriptor, create new if needed. */
-static struct alloc_pool_descriptor *
+struct alloc_pool_descriptor *
allocate_pool_descriptor (const char *name)
{
if
Re: [PATCH 01/35] Introduce new type-based pool allocator.
On 05/29/2015 07:31 AM, Martin Liška wrote: On 05/28/2015 07:15 PM, Jeff Law wrote: On 05/28/2015 06:49 AM, Martin Liška wrote: . This mechanism has been just adapted. I find it quite useful as we have examples in source code where we allocate same struct/class types from a various pool. For debugging purpose, it helps to identify if release operation is called for a correct pool. I saw that you were following existing practice for the pools in the removal patch. I still don't like it as it makes mixing and matching objects harder when debugging gcc and if the structure is exposed for plugins, then we've got an unnecessary ABI plugin breakage. I certainly understand how it's useful -- I'm not questioning that. I'm questioning changing the size of structures on ENABLE_CHECKING. My first inclination would be to include all that stuff unconditionally. If that's too much overhead, then perhaps include the structure member, but not bother with any of the bookkeeping except for ENABLE_CHECKING. Hi. Updated version of patch removes ENABLE_CHECKING in the struct definition. News in the patchset I'm going to re-send: + Changelog entries are fixed for spaces + Each patch passes ./contrib/check_GNU_style.sh script + pool_allocator::pool_allocator is a trivial constructor and first allocation launches initialization + some patches are squashed as were mentioned multiple time (ira-color.c, ira-build.c) The patch set survives x86_64-linux-pc boostrap, I'm going to re-run regression tests. Sounds perfect. Once the regression testing is done, the whole set is fine for the trunk. Thanks for tackling this. jeff
Re: [PATCH 01/35] Introduce new type-based pool allocator.
On 05/28/2015 06:49 AM, Martin Liška wrote: . This mechanism has been just adapted. I find it quite useful as we have examples in source code where we allocate same struct/class types from a various pool. For debugging purpose, it helps to identify if release operation is called for a correct pool. I saw that you were following existing practice for the pools in the removal patch. I still don't like it as it makes mixing and matching objects harder when debugging gcc and if the structure is exposed for plugins, then we've got an unnecessary ABI plugin breakage. I certainly understand how it's useful -- I'm not questioning that. I'm questioning changing the size of structures on ENABLE_CHECKING. My first inclination would be to include all that stuff unconditionally. If that's too much overhead, then perhaps include the structure member, but not bother with any of the bookkeeping except for ENABLE_CHECKING. Anyway, I would like to commit all these patches at once (one by one). Thus, I'm going to wait for approval for the whole series before I'll commit the set. Quite reasonable -- I was mostly trying to make sure I understood the testing situation. I think at this point the whole series is approved, so you can move forward. jeff
Re: [PATCH 01/35] Introduce new type-based pool allocator.
On 05/27/2015 07:44 PM, Jeff Law wrote:
On 05/27/2015 07:56 AM, mliska wrote:
Hello.
Following patch set attempts to replace old-style pool allocator
to a type-based one. Moreover, as we utilize classes and structs that are used
just by a pool allocator, these types have overwritten ctors and dtors.
Thus, using the allocator is much easier and we shouldn't cast types
back and forth. Another beneficat can be achieved in future, as we will
be able to call a class constructors to correctly register a location,
where a memory is allocated (-fgather-detailed-mem-stats).
Patch can boostrap on x86_64-linux-gnu and ppc64-linux-gnu and
survives regression tests on x86_64-linux-gnu.
Ready for trunk?
Thanks,
Martin
gcc/ChangeLog:
2015-04-30 Martin Liska mli...@suse.cz
* alloc-pool.c (struct alloc_pool_descriptor): Move definition
to header file.
* alloc-pool.h (pool_allocator::pool_allocator): New function.
(pool_allocator::release): Likewise.
(inline pool_allocator::release_if_empty): Likewise.
(inline pool_allocator::~pool_allocator): Likewise.
(pool_allocator::allocate): Likewise.
(pool_allocator::remove): Likewise.
So on a general note, I don't like changing the size of the structure based on
ENABLE_CHECKING. If we've got other cases where we do this, then I guess it's
OK, but if not, I'd prefer not to start doing so.
Hello.
This mechanism has been just adapted. I find it quite useful as we have
examples in source code where we
allocate same struct/class types from a various pool. For debugging purpose, it
helps to identify if
release operation is called for a correct pool.
---
+
+ /* Align X to 8. */
+ size_t align_eight (size_t x)
+ {
+return (((x+7) 3) 3);
+ }
+
+ const char *m_name;
+#ifdef ENABLE_CHECKING
+ ALLOC_POOL_ID_TYPE m_id;
+#endif
+ size_t m_elts_per_block;
+
+ /* These are the elements that have been allocated at least once and freed.
*/
+ allocation_pool_list *m_returned_free_list;
+
+ /* These are the elements that have not yet been allocated out of
+ the last block obtained from XNEWVEC. */
+ char* m_virgin_free_list;
+
+ /* The number of elements in the virgin_free_list that can be
+ allocated before needing another block. */
+ size_t m_virgin_elts_remaining;
+ size_t m_elts_allocated;
+ size_t m_elts_free;
+ size_t m_blocks_allocated;
+ allocation_pool_list *m_block_list;
+ size_t m_block_size;
+ size_t m_elt_size;
Several fields aren't documented. They're largely self-explanatory, so I won't
insist you document those trailing fields. Your call whether or not to add
docs for them.
Ok, even tough they are self-explanatory, I'm going to document these fields.
+
+ /* Now align the size to a multiple of 4. */
+ size = align_eight (size);
Why not just aligned to 4, rather than a multiple of 4? Presumably the extra 4
bytes don't matter in practice?
Also adapted constant, hope it's chosen as the best.
+
+template typename T
+void
+inline pool_allocatorT::release_if_empty ()
+{
+ if (m_elts_free == m_elts_allocated)
+release ();
+}
Is the release_if_empty all that useful in practice?
Yes, 02/x uses that feature.
So the big issue in my mind continues to be the additional element in the
structure when ENABLE_CHECKING is on. As mentioned earlier, if we're already
doing this elsewhere, then I won't object. If we aren't, then I don't want to
start doing so now.
The rest of the stuff are just minor questions, but nothing which would in my
mind stop this from going forward.
Presumably your testing was with the whole series and they can't go in
piecemeal, right?
Right, regression tests were run just once for the whole series, but I've
tested that every individual patch can be applied and the compiler can be
successfully built.
Anyway, I would like to commit all these patches at once (one by one).
Thus, I'm going to wait for approval for the whole series before I'll commit
the set.
Thanks,
Martin
jeff
[PATCH 01/35] Introduce new type-based pool allocator.
Hello.
Following patch set attempts to replace old-style pool allocator
to a type-based one. Moreover, as we utilize classes and structs that are used
just by a pool allocator, these types have overwritten ctors and dtors.
Thus, using the allocator is much easier and we shouldn't cast types
back and forth. Another beneficat can be achieved in future, as we will
be able to call a class constructors to correctly register a location,
where a memory is allocated (-fgather-detailed-mem-stats).
Patch can boostrap on x86_64-linux-gnu and ppc64-linux-gnu and
survives regression tests on x86_64-linux-gnu.
Ready for trunk?
Thanks,
Martin
gcc/ChangeLog:
2015-04-30 Martin Liska mli...@suse.cz
* alloc-pool.c (struct alloc_pool_descriptor): Move definition
to header file.
* alloc-pool.h (pool_allocator::pool_allocator): New function.
(pool_allocator::release): Likewise.
(inline pool_allocator::release_if_empty): Likewise.
(inline pool_allocator::~pool_allocator): Likewise.
(pool_allocator::allocate): Likewise.
(pool_allocator::remove): Likewise.
---
gcc/alloc-pool.c | 33 +-
gcc/alloc-pool.h | 350 +++
2 files changed, 355 insertions(+), 28 deletions(-)
diff --git a/gcc/alloc-pool.c b/gcc/alloc-pool.c
index 81909d8..0bea7a6 100644
--- a/gcc/alloc-pool.c
+++ b/gcc/alloc-pool.c
@@ -25,6 +25,8 @@ along with GCC; see the file COPYING3. If not see
#include hash-table.h
#include hash-map.h
+ALLOC_POOL_ID_TYPE last_id;
+
#define align_eight(x) (((x+7) 3) 3)
/* The internal allocation object. */
@@ -58,36 +60,10 @@ typedef struct allocation_object_def
#define USER_PTR_FROM_ALLOCATION_OBJECT_PTR(X) \
((void *) (((allocation_object *) (X))-u.data))
-#ifdef ENABLE_CHECKING
-/* Last used ID. */
-static ALLOC_POOL_ID_TYPE last_id;
-#endif
-
-/* Store information about each particular alloc_pool. Note that this
- will underestimate the amount the amount of storage used by a small amount:
- 1) The overhead in a pool is not accounted for.
- 2) The unallocated elements in a block are not accounted for. Note
- that this can at worst case be one element smaller that the block
- size for that pool. */
-struct alloc_pool_descriptor
-{
- /* Number of pools allocated. */
- unsigned long created;
- /* Gross allocated storage. */
- unsigned long allocated;
- /* Amount of currently active storage. */
- unsigned long current;
- /* Peak amount of storage used. */
- unsigned long peak;
- /* Size of element in the pool. */
- int elt_size;
-};
-
/* Hashtable mapping alloc_pool names to descriptors. */
-static hash_mapconst char *, alloc_pool_descriptor *alloc_pool_hash;
+hash_mapconst char *, alloc_pool_descriptor *alloc_pool_hash;
-/* For given name, return descriptor, create new if needed. */
-static struct alloc_pool_descriptor *
+struct alloc_pool_descriptor *
allocate_pool_descriptor (const char *name)
{
if (!alloc_pool_hash)
@@ -96,6 +72,7 @@ allocate_pool_descriptor (const char *name)
return alloc_pool_hash-get_or_insert (name);
}
+
/* Create a pool of things of size SIZE, with NUM in each block we
allocate. */
diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
index 0c30711..8fd664f 100644
--- a/gcc/alloc-pool.h
+++ b/gcc/alloc-pool.h
@@ -20,6 +20,8 @@ along with GCC; see the file COPYING3. If not see
#ifndef ALLOC_POOL_H
#define ALLOC_POOL_H
+#include hash-map.h
+
typedef unsigned long ALLOC_POOL_ID_TYPE;
typedef struct alloc_pool_list_def
@@ -63,4 +65,352 @@ extern void free_alloc_pool_if_empty (alloc_pool *);
extern void *pool_alloc (alloc_pool) ATTRIBUTE_MALLOC;
extern void pool_free (alloc_pool, void *);
extern void dump_alloc_pool_statistics (void);
+
+typedef unsigned long ALLOC_POOL_ID_TYPE;
+
+/* Type based memory pool allocator. */
+template typename T
+class pool_allocator
+{
+public:
+ /* Default constructor for pool allocator called NAME. Each block
+ has NUM elements. The allocator support EXTRA_SIZE and can
+ potentially IGNORE_TYPE_SIZE. */
+ pool_allocator (const char *name, size_t num, size_t extra_size = 0,
+ bool ignore_type_size = false);
+
+ /* Default destuctor. */
+ ~pool_allocator ();
+
+ /* Release internal data structures. */
+ void release ();
+
+ /* Release internal data structures if the pool has not allocated
+ an object. */
+ void release_if_empty ();
+
+ /* Allocate a new object. */
+ T *allocate () ATTRIBUTE_MALLOC;
+
+ /* Release OBJECT that must come from the pool. */
+ void remove (T *object);
+
+private:
+ struct allocation_pool_list
+ {
+allocation_pool_list *next;
+ };
+
+ template typename U
+ struct allocation_object
+ {
+#ifdef ENABLE_CHECKING
+/* The ID of alloc pool which the object was allocated from. */
+ALLOC_POOL_ID_TYPE id;
+#endif
+
+union
+ {
+ /* The data of the object. */
+
Re: [PATCH 01/35] Introduce new type-based pool allocator.
On 05/27/2015 07:56 AM, mliska wrote:
Hello.
Following patch set attempts to replace old-style pool allocator
to a type-based one. Moreover, as we utilize classes and structs that are used
just by a pool allocator, these types have overwritten ctors and dtors.
Thus, using the allocator is much easier and we shouldn't cast types
back and forth. Another beneficat can be achieved in future, as we will
be able to call a class constructors to correctly register a location,
where a memory is allocated (-fgather-detailed-mem-stats).
Patch can boostrap on x86_64-linux-gnu and ppc64-linux-gnu and
survives regression tests on x86_64-linux-gnu.
Ready for trunk?
Thanks,
Martin
gcc/ChangeLog:
2015-04-30 Martin Liska mli...@suse.cz
* alloc-pool.c (struct alloc_pool_descriptor): Move definition
to header file.
* alloc-pool.h (pool_allocator::pool_allocator): New function.
(pool_allocator::release): Likewise.
(inline pool_allocator::release_if_empty): Likewise.
(inline pool_allocator::~pool_allocator): Likewise.
(pool_allocator::allocate): Likewise.
(pool_allocator::remove): Likewise.
So on a general note, I don't like changing the size of the structure
based on ENABLE_CHECKING. If we've got other cases where we do this,
then I guess it's OK, but if not, I'd prefer not to start doing so.
---
+
+ /* Align X to 8. */
+ size_t align_eight (size_t x)
+ {
+return (((x+7) 3) 3);
+ }
+
+ const char *m_name;
+#ifdef ENABLE_CHECKING
+ ALLOC_POOL_ID_TYPE m_id;
+#endif
+ size_t m_elts_per_block;
+
+ /* These are the elements that have been allocated at least once and freed.
*/
+ allocation_pool_list *m_returned_free_list;
+
+ /* These are the elements that have not yet been allocated out of
+ the last block obtained from XNEWVEC. */
+ char* m_virgin_free_list;
+
+ /* The number of elements in the virgin_free_list that can be
+ allocated before needing another block. */
+ size_t m_virgin_elts_remaining;
+ size_t m_elts_allocated;
+ size_t m_elts_free;
+ size_t m_blocks_allocated;
+ allocation_pool_list *m_block_list;
+ size_t m_block_size;
+ size_t m_elt_size;
Several fields aren't documented. They're largely self-explanatory, so
I won't insist you document those trailing fields. Your call whether or
not to add docs for them.
+
+ /* Now align the size to a multiple of 4. */
+ size = align_eight (size);
Why not just aligned to 4, rather than a multiple of 4? Presumably the
extra 4 bytes don't matter in practice?
+
+template typename T
+void
+inline pool_allocatorT::release_if_empty ()
+{
+ if (m_elts_free == m_elts_allocated)
+release ();
+}
Is the release_if_empty all that useful in practice?
So the big issue in my mind continues to be the additional element in
the structure when ENABLE_CHECKING is on. As mentioned earlier, if
we're already doing this elsewhere, then I won't object. If we aren't,
then I don't want to start doing so now.
The rest of the stuff are just minor questions, but nothing which would
in my mind stop this from going forward.
Presumably your testing was with the whole series and they can't go in
piecemeal, right?
jeff
