Re: Statically-allocated objects with non-trivial ctors (was Re: [PATCH 33/35] Change use to type-based pool allocator in ira-color.c.)

2015-05-28 Thread Trevor Saunders 
On Thu, May 28, 2015 at 08:47:16PM +0200, Martin Liška wrote:
 On 05/28/2015 08:03 PM, Jakub Jelinek wrote:
 On Thu, May 28, 2015 at 07:57:39PM +0200, Richard Biener wrote:
 But we've been trying to avoid this. And the jit might not be too happy 
 about it either.
 
 Yeah, we should certainly try to avoid them, especially if it would affect
 many variables having to be constructed.
 
  Jakub
 
 
 Ok, thus I will do it as before my modifications:
 
 static pool_allocator update_cost_record *update_cost_record_pool = NULL;
 
 /* Initiate update cost records.  */
 static void
 init_update_cost_records (void)
 {
  update_cost_record_pool = new pool_allocator update_cost_record
(update cost records, 100);
 }
 
 I'm going to migrate rest of patches that use the same construct.


Hrm, why not just change pool_allocator so it does the first allocation
on the first alloc and just initializes everything to null / 0?  Then
the ctor would be close to trivial.  Then if you really care about the
stuff gcc doesn't optimize away you could add a special class
static_pool_allocator (you might also need to hack in a way to get the
c++ fe to do constexpr / defaulted functions).

Trev

 
 Thanks,
 Martin

Re: Statically-allocated objects with non-trivial ctors (was Re: [PATCH 33/35] Change use to type-based pool allocator in ira-color.c.)

2015-05-28 Thread Richard Biener 
On May 28, 2015 7:06:36 PM GMT+02:00, Jeff Law l...@redhat.com wrote:
On 05/28/2015 04:42 AM, David Malcolm wrote:

 Am I right in thinking that this is a statically-allocated object
with a
 non-trivial constructor?  i.e. that this constructor has to run
before
 main is entered?

 Do our coding guidelines allow for this?  (I've been burned by this
 before, on a buggy C++ runtime that didn't manage to support these).
 I'm a little nervous about this, touching global state before
 main (e.g. from the point-of-view of the JIT), though I don't know
yet
 if this is just a gut reaction, or if there's a valid concern here
(I'm
 officially on holiday this week, so I haven't had a chance to dig
deeply
 into these patches yet, sorry).
That idiom is used in various places by Martin's patches.   I didn't
see 
a strong rhyme or reason behind why it was used over allocating 
something in automatic or heap storage.

As to supporting it, I'm not terribly concerned about other buggy C++ 
runtimes.  GCC bootstraps with GCC, which means we've got our C++ 
runtime.  The only worry becomes the low level bits that we build our 
static ctor/dtor support on top of -- and I haven't seen major problems

with that for eons.

But we've been trying to avoid this. And the jit might not be too happy about 
it either.

jeff

Re: Statically-allocated objects with non-trivial ctors (was Re: [PATCH 33/35] Change use to type-based pool allocator in ira-color.c.)

2015-05-28 Thread Jakub Jelinek 
On Thu, May 28, 2015 at 07:57:39PM +0200, Richard Biener wrote:
 But we've been trying to avoid this. And the jit might not be too happy about 
 it either.

Yeah, we should certainly try to avoid them, especially if it would affect
many variables having to be constructed.

Jakub

Re: Statically-allocated objects with non-trivial ctors (was Re: [PATCH 33/35] Change use to type-based pool allocator in ira-color.c.)

2015-05-28 Thread Martin Liška 

On 05/28/2015 08:03 PM, Jakub Jelinek wrote:

On Thu, May 28, 2015 at 07:57:39PM +0200, Richard Biener wrote:

But we've been trying to avoid this. And the jit might not be too happy about 
it either.


Yeah, we should certainly try to avoid them, especially if it would affect
many variables having to be constructed.

Jakub



Ok, thus I will do it as before my modifications:

static pool_allocator update_cost_record *update_cost_record_pool = NULL;

/* Initiate update cost records.  */
static void
init_update_cost_records (void)
{
 update_cost_record_pool = new pool_allocator update_cost_record
   (update cost records, 100);
}

I'm going to migrate rest of patches that use the same construct.

Thanks,
Martin

Re: Statically-allocated objects with non-trivial ctors (was Re: [PATCH 33/35] Change use to type-based pool allocator in ira-color.c.)

2015-05-28 Thread Jeff Law 

On 05/28/2015 04:42 AM, David Malcolm wrote:


Am I right in thinking that this is a statically-allocated object with a
non-trivial constructor?  i.e. that this constructor has to run before
main is entered?

Do our coding guidelines allow for this?  (I've been burned by this
before, on a buggy C++ runtime that didn't manage to support these).
I'm a little nervous about this, touching global state before
main (e.g. from the point-of-view of the JIT), though I don't know yet
if this is just a gut reaction, or if there's a valid concern here (I'm
officially on holiday this week, so I haven't had a chance to dig deeply
into these patches yet, sorry).
That idiom is used in various places by Martin's patches.   I didn't see 
a strong rhyme or reason behind why it was used over allocating 
something in automatic or heap storage.


As to supporting it, I'm not terribly concerned about other buggy C++ 
runtimes.  GCC bootstraps with GCC, which means we've got our C++ 
runtime.  The only worry becomes the low level bits that we build our 
static ctor/dtor support on top of -- and I haven't seen major problems 
with that for eons.


jeff

Re: Statically-allocated objects with non-trivial ctors (was Re: [PATCH 33/35] Change use to type-based pool allocator in ira-color.c.)

2015-05-28 Thread Trevor Saunders 
On Thu, May 28, 2015 at 06:42:57AM -0400, David Malcolm wrote:
 On Wed, 2015-05-27 at 15:56 +0200, mliska wrote:
  gcc/ChangeLog:
  
  2015-04-30  Martin Liska  mli...@suse.cz
  
  * ira-color.c (init_update_cost_records): Use new type-based pool 
  allocator.
  (get_update_cost_record): Likewise.
  (free_update_cost_record_list): Likewise.
  (finish_update_cost_records): Likewise.
  (initiate_cost_update): Likewise.
  ---
   gcc/ira-color.c | 19 +--
   1 file changed, 5 insertions(+), 14 deletions(-)
  
  diff --git a/gcc/ira-color.c b/gcc/ira-color.c
  index 4750714..4aec98e 100644
  --- a/gcc/ira-color.c
  +++ b/gcc/ira-color.c
  @@ -1166,16 +1166,8 @@ setup_profitable_hard_regs (void)
  allocnos.  */
   
   /* Pool for update cost records.  */
  -static alloc_pool update_cost_record_pool;
  -
  -/* Initiate update cost records.  */
  -static void
  -init_update_cost_records (void)
  -{
  -  update_cost_record_pool
  -= create_alloc_pool (update cost records,
  -sizeof (struct update_cost_record), 100);
  -}
  +static pool_allocatorupdate_cost_record update_cost_record_pool
  +  (update cost records, 100);
 
 Am I right in thinking that this is a statically-allocated object with a
 non-trivial constructor?  i.e. that this constructor has to run before
 main is entered?

yes though I think it'd be pretty easy to make it basically trivial but
with a static initializer because gcc doesn't optimize them well, and
with a bit more work we could probably get rid of the static initializer
without actually fixing gcc.

 Do our coding guidelines allow for this?  (I've been burned by this
 before, on a buggy C++ runtime that didn't manage to support these).

I'm pretty sure there already are some iirc the pretty printers are one
example.

 I'm a little nervous about this, touching global state before
 main (e.g. from the point-of-view of the JIT), though I don't know yet
 if this is just a gut reaction, or if there's a valid concern here (I'm

afaik it should work fine.  Of course this is global data which isn't
great, but that's a preexisting problem.

Trev

Statically-allocated objects with non-trivial ctors (was Re: [PATCH 33/35] Change use to type-based pool allocator in ira-color.c.)

2015-05-28 Thread David Malcolm 
On Wed, 2015-05-27 at 15:56 +0200, mliska wrote:
 gcc/ChangeLog:
 
 2015-04-30  Martin Liska  mli...@suse.cz
 
   * ira-color.c (init_update_cost_records): Use new type-based pool 
 allocator.
   (get_update_cost_record): Likewise.
   (free_update_cost_record_list): Likewise.
   (finish_update_cost_records): Likewise.
   (initiate_cost_update): Likewise.
 ---
  gcc/ira-color.c | 19 +--
  1 file changed, 5 insertions(+), 14 deletions(-)
 
 diff --git a/gcc/ira-color.c b/gcc/ira-color.c
 index 4750714..4aec98e 100644
 --- a/gcc/ira-color.c
 +++ b/gcc/ira-color.c
 @@ -1166,16 +1166,8 @@ setup_profitable_hard_regs (void)
 allocnos.  */
  
  /* Pool for update cost records.  */
 -static alloc_pool update_cost_record_pool;
 -
 -/* Initiate update cost records.  */
 -static void
 -init_update_cost_records (void)
 -{
 -  update_cost_record_pool
 -= create_alloc_pool (update cost records,
 -  sizeof (struct update_cost_record), 100);
 -}
 +static pool_allocatorupdate_cost_record update_cost_record_pool
 +  (update cost records, 100);

Am I right in thinking that this is a statically-allocated object with a
non-trivial constructor?  i.e. that this constructor has to run before
main is entered?

Do our coding guidelines allow for this?  (I've been burned by this
before, on a buggy C++ runtime that didn't manage to support these).
I'm a little nervous about this, touching global state before
main (e.g. from the point-of-view of the JIT), though I don't know yet
if this is just a gut reaction, or if there's a valid concern here (I'm
officially on holiday this week, so I haven't had a chance to dig deeply
into these patches yet, sorry).

[...snip...]

 @@ -1264,7 +1256,6 @@ initiate_cost_update (void)
  = (struct update_cost_queue_elem *) ira_allocate (size);
memset (update_cost_queue_elems, 0, size);
update_cost_check = 0;
 -  init_update_cost_records ();
  }

(for reference, this is where the manually-coded initialization call was
made)

Hope this is constructive
Dave

Re: [PATCH 33/35] Change use to type-based pool allocator in ira-color.c.

2015-05-27 Thread Jeff Law 

On 05/27/2015 07:56 AM, mliska wrote:

gcc/ChangeLog:

2015-04-30  Martin Liska  mli...@suse.cz

* ira-color.c (init_update_cost_records): Use new type-based pool 
allocator.
(get_update_cost_record): Likewise.
(free_update_cost_record_list): Likewise.
(finish_update_cost_records): Likewise.
(initiate_cost_update): Likewise.

OK.
Jeff

[PATCH 33/35] Change use to type-based pool allocator in ira-color.c.

2015-05-27 Thread mliska 
gcc/ChangeLog:

2015-04-30  Martin Liska  mli...@suse.cz

* ira-color.c (init_update_cost_records): Use new type-based pool 
allocator.
(get_update_cost_record): Likewise.
(free_update_cost_record_list): Likewise.
(finish_update_cost_records): Likewise.
(initiate_cost_update): Likewise.
---
 gcc/ira-color.c | 19 +--
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/gcc/ira-color.c b/gcc/ira-color.c
index 4750714..4aec98e 100644
--- a/gcc/ira-color.c
+++ b/gcc/ira-color.c
@@ -1166,16 +1166,8 @@ setup_profitable_hard_regs (void)
allocnos.  */
 
 /* Pool for update cost records.  */
-static alloc_pool update_cost_record_pool;
-
-/* Initiate update cost records.  */
-static void
-init_update_cost_records (void)
-{
-  update_cost_record_pool
-= create_alloc_pool (update cost records,
-sizeof (struct update_cost_record), 100);
-}
+static pool_allocatorupdate_cost_record update_cost_record_pool
+  (update cost records, 100);
 
 /* Return new update cost record with given params.  */
 static struct update_cost_record *
@@ -1184,7 +1176,7 @@ get_update_cost_record (int hard_regno, int divisor,
 {
   struct update_cost_record *record;
 
-  record = (struct update_cost_record *) pool_alloc (update_cost_record_pool);
+  record = update_cost_record_pool.allocate ();
   record-hard_regno = hard_regno;
   record-divisor = divisor;
   record-next = next;
@@ -1200,7 +1192,7 @@ free_update_cost_record_list (struct update_cost_record 
*list)
   while (list != NULL)
 {
   next = list-next;
-  pool_free (update_cost_record_pool, list);
+  update_cost_record_pool.remove (list);
   list = next;
 }
 }
@@ -1209,7 +1201,7 @@ free_update_cost_record_list (struct update_cost_record 
*list)
 static void
 finish_update_cost_records (void)
 {
-  free_alloc_pool (update_cost_record_pool);
+  update_cost_record_pool.release ();
 }
 
 /* Array whose element value is TRUE if the corresponding hard
@@ -1264,7 +1256,6 @@ initiate_cost_update (void)
 = (struct update_cost_queue_elem *) ira_allocate (size);
   memset (update_cost_queue_elems, 0, size);
   update_cost_check = 0;
-  init_update_cost_records ();
 }
 
 /* Deallocate data used by function update_costs_from_copies.  */
-- 
2.1.4