Re: [RFC PATCH] add auto_bitmap
On Thu, Nov 14, 2013 at 10:33 PM, Jeff Law l...@redhat.com wrote: On 11/14/13 14:14, Richard Biener wrote: I'm just pointed out that of all the stuff you changed, these were the only ones I saw where lifetimes were changed significantly. I still ask why we need a new type and cannot put this functionality into bitmap_head itself. Given that bitmap is just a *bitmap_head_def aren't we suggesting the same thing? Not sure - I thought Trevor wanted to make auto_bitmap a full C++ thing, not bitmap itself? Richard. jeff
Re: [RFC PATCH] add auto_bitmap
On Fri, Nov 15, 2013 at 10:56:24AM +0100, Richard Biener wrote: On Thu, Nov 14, 2013 at 10:33 PM, Jeff Law l...@redhat.com wrote: On 11/14/13 14:14, Richard Biener wrote: I'm just pointed out that of all the stuff you changed, these were the only ones I saw where lifetimes were changed significantly. I still ask why we need a new type and cannot put this functionality into bitmap_head itself. Given that bitmap is just a *bitmap_head_def aren't we suggesting the same thing? Not sure - I thought Trevor wanted to make auto_bitmap a full C++ thing, not bitmap itself? My only firm goals are less manual memory management, and moving the bitmap_head bit onto the stack would be really nice. I'd also like to leave bitmaps allocated in gc memory alone for the time being, but those are the only firm goals. I'm currently trying the approach of adding constructors and destructors to bitmap_head, but apparently something is causing them to get invoked even when everybody deals with bitmap_head * which leads to ICEs that I'm investigating now. Trev Richard. jeff
Re: [RFC PATCH] add auto_bitmap
On Fri, Nov 15, 2013 at 11:37 AM, Trevor Saunders tsaund...@mozilla.com wrote: On Fri, Nov 15, 2013 at 10:56:24AM +0100, Richard Biener wrote: On Thu, Nov 14, 2013 at 10:33 PM, Jeff Law l...@redhat.com wrote: On 11/14/13 14:14, Richard Biener wrote: I'm just pointed out that of all the stuff you changed, these were the only ones I saw where lifetimes were changed significantly. I still ask why we need a new type and cannot put this functionality into bitmap_head itself. Given that bitmap is just a *bitmap_head_def aren't we suggesting the same thing? Not sure - I thought Trevor wanted to make auto_bitmap a full C++ thing, not bitmap itself? My only firm goals are less manual memory management, and moving the bitmap_head bit onto the stack would be really nice. I'd also like to leave bitmaps allocated in gc memory alone for the time being, but those are the only firm goals. I'm currently trying the approach of adding constructors and destructors to bitmap_head, but apparently something is causing them to get invoked even when everybody deals with bitmap_head * which leads to ICEs that I'm investigating now. They are used embedded into other structures as well (to avoid a pointer indirection). Richard. Trev Richard. jeff
Re: [RFC PATCH] add auto_bitmap
On Fri, Nov 15, 2013 at 12:11:07PM +0100, Richard Biener wrote: On Fri, Nov 15, 2013 at 11:37 AM, Trevor Saunders tsaund...@mozilla.com wrote: On Fri, Nov 15, 2013 at 10:56:24AM +0100, Richard Biener wrote: On Thu, Nov 14, 2013 at 10:33 PM, Jeff Law l...@redhat.com wrote: On 11/14/13 14:14, Richard Biener wrote: I'm just pointed out that of all the stuff you changed, these were the only ones I saw where lifetimes were changed significantly. I still ask why we need a new type and cannot put this functionality into bitmap_head itself. Given that bitmap is just a *bitmap_head_def aren't we suggesting the same thing? Not sure - I thought Trevor wanted to make auto_bitmap a full C++ thing, not bitmap itself? My only firm goals are less manual memory management, and moving the bitmap_head bit onto the stack would be really nice. I'd also like to leave bitmaps allocated in gc memory alone for the time being, but those are the only firm goals. I'm currently trying the approach of adding constructors and destructors to bitmap_head, but apparently something is causing them to get invoked even when everybody deals with bitmap_head * which leads to ICEs that I'm investigating now. They are used embedded into other structures as well (to avoid a pointer indirection). yeah, I see that now, but actually what seems to haave een causing the problem is that df-core.c df-problems.c and tree-ssa-pre.c where already putting them directly on the stack. That and the fact bitmap_clear doesn't make head-first NULL, and so calling bitmap_clear twice on the same bitmap causes a double free. Trev Richard. Trev Richard. jeff
Re: [RFC PATCH] add auto_bitmap
On Thu, Nov 14, 2013 at 12:04 PM, tsaund...@mozilla.com wrote:
From: Trevor Saunders tsaund...@mozilla.com
Hi,
this patch adds and starts to use a class auto_bitmap, which is a very thin
wrapper around bitmap. Its advantage is that it takes care of delocation
automatically. So you can do things like
int
f ()
{
auto_bitmap x;
// do stuff with x
}
Another advantage of this class is it puts the bitmap_head struct on the stack
instead of mallocing it or using a obstack.
Hm, but then eventually you increase the lifetime of the bitmap
until the scope closes.
Why not give bitmap_head a constructor/destructor and allow
auto use of that. Isn't that exactly what should get 'auto' handling
automagically?
Richard.
I Think the biggest question is if I should make auto_bitmap a full c++ified
wrapper around bitmap or if I should contiune just taking the address of it
and passing it as a bitmap, but other comments are of course welcome too.
Trev
diff --git a/gcc/bitmap.h b/gcc/bitmap.h
index b3cb5da..78901fa 100644
--- a/gcc/bitmap.h
+++ b/gcc/bitmap.h
@@ -312,6 +312,14 @@ extern hashval_t bitmap_hash (const_bitmap);
#define BITMAP_FREE(BITMAP) \
((void) (bitmap_obstack_free ((bitmap) BITMAP), (BITMAP) = (bitmap)
NULL))
+/* bitmap with automatic management of resources. */
+class auto_bitmap : public bitmap_head_def
+{
+public:
+ auto_bitmap (bitmap_obstack *o = bitmap_default_obstack) {
bitmap_initialize_stat (this, o); }
+ ~auto_bitmap () { bitmap_clear (this); }
+};
+
/* Iterator for bitmaps. */
typedef struct
diff --git a/gcc/bt-load.c b/gcc/bt-load.c
index 5384d01..819bcbb 100644
--- a/gcc/bt-load.c
+++ b/gcc/bt-load.c
@@ -1072,7 +1072,7 @@ combine_btr_defs (btr_def def, HARD_REG_SET
*btrs_live_in_range)
target registers live over the merged range. */
int btr;
HARD_REG_SET combined_btrs_live;
- bitmap combined_live_range = BITMAP_ALLOC (NULL);
+ auto_bitmap combined_live_range;
btr_user user;
if (other_def-live_range == NULL)
@@ -1081,10 +1081,10 @@ combine_btr_defs (btr_def def, HARD_REG_SET
*btrs_live_in_range)
btr_def_live_range (other_def, dummy_btrs_live_in_range);
}
COPY_HARD_REG_SET (combined_btrs_live, *btrs_live_in_range);
- bitmap_copy (combined_live_range, def-live_range);
+ bitmap_copy (combined_live_range, def-live_range);
for (user = other_def-uses; user != NULL; user = user-next)
- augment_live_range (combined_live_range, combined_btrs_live,
+ augment_live_range (combined_live_range, combined_btrs_live,
def-bb, user-bb,
(flag_btr_bb_exclusive
|| user-insn != BB_END (def-bb)
@@ -1121,7 +1121,7 @@ combine_btr_defs (btr_def def, HARD_REG_SET
*btrs_live_in_range)
REGNO (user-use)));
clear_btr_from_live_range (other_def);
other_def-uses = NULL;
- bitmap_copy (def-live_range, combined_live_range);
+ bitmap_copy (def-live_range, combined_live_range);
if (other_def-btr == btr
other_def-other_btr_uses_after_use)
def-other_btr_uses_after_use = 1;
COPY_HARD_REG_SET (*btrs_live_in_range, combined_btrs_live);
@@ -1130,7 +1130,6 @@ combine_btr_defs (btr_def def, HARD_REG_SET
*btrs_live_in_range)
delete_insn (other_def-insn);
}
- BITMAP_FREE (combined_live_range);
}
}
}
diff --git a/gcc/cfgloop.c b/gcc/cfgloop.c
index 3ff8e84..4510c0b 100644
--- a/gcc/cfgloop.c
+++ b/gcc/cfgloop.c
@@ -917,7 +917,6 @@ get_loop_body_in_bfs_order (const struct loop *loop)
{
basic_block *blocks;
basic_block bb;
- bitmap visited;
unsigned int i = 0;
unsigned int vc = 1;
@@ -925,7 +924,7 @@ get_loop_body_in_bfs_order (const struct loop *loop)
gcc_assert (loop-latch != EXIT_BLOCK_PTR);
blocks = XNEWVEC (basic_block, loop-num_nodes);
- visited = BITMAP_ALLOC (NULL);
+ auto_bitmap visited;
bb = loop-header;
while (i loop-num_nodes)
@@ -933,7 +932,7 @@ get_loop_body_in_bfs_order (const struct loop *loop)
edge e;
edge_iterator ei;
- if (bitmap_set_bit (visited, bb-index))
+ if (bitmap_set_bit (visited, bb-index))
/* This basic block is now visited */
blocks[i++] = bb;
@@ -941,7 +940,7 @@ get_loop_body_in_bfs_order (const struct loop *loop)
{
if (flow_bb_inside_loop_p (loop, e-dest))
{
- if (bitmap_set_bit (visited, e-dest-index))
+ if (bitmap_set_bit (visited, e-dest-index))
blocks[i++] = e-dest;
}
}
@@ -951,7 +950,6 @@ get_loop_body_in_bfs_order (const struct loop *loop)
bb =
Re: [RFC PATCH] add auto_bitmap
Hi, On Thu, 14 Nov 2013, Richard Biener wrote: Why not give bitmap_head a constructor/destructor and allow auto use of that. Isn't that exactly what should get 'auto' handling automagically? auto != c++98 :-/ Ciao, Michael.
Re: [RFC PATCH] add auto_bitmap
On 11/14/13 04:04, tsaund...@mozilla.com wrote:
From: Trevor Saunders tsaund...@mozilla.com
Hi,
this patch adds and starts to use a class auto_bitmap, which is a very thin
wrapper around bitmap. Its advantage is that it takes care of delocation
automatically. So you can do things like
int
f ()
{
auto_bitmap x;
// do stuff with x
}
Another advantage of this class is it puts the bitmap_head struct on the stack
instead of mallocing it or using a obstack.
I Think the biggest question is if I should make auto_bitmap a full c++ified
wrapper around bitmap or if I should contiune just taking the address of it
and passing it as a bitmap, but other comments are of course welcome too.
I'd prefer to see it fully c++ified.
In response to one of Richi's comments, I spot checked the patch and
only found two occurrences where this lengthened the lifetime of the
bitmap in any significant way. The vast majority of the time any
increase in length was trivial.
Those instances are in tree-ssa-loop-ivopts.c and the other in
tree-ssa-strlen.c. I don't think you need to change anything for them,
I'm just pointed out that of all the stuff you changed, these were the
only ones I saw where lifetimes were changed significantly.
jeff
Re: [RFC PATCH] add auto_bitmap
On 11/14/13 07:52, Richard Biener wrote: Another advantage of this class is it puts the bitmap_head struct on the stack instead of mallocing it or using a obstack. Hm, but then eventually you increase the lifetime of the bitmap until the scope closes. Yea, but often that's when we're releasing them anyway ;-) I don't think we've gone to too much trouble to try and release the bitmaps as soon as they're no longer needed. Releasing at end of scope seems fine to me. I'm a fan of RAII style code. It's less error prone and often results in code that is easier to understand because the code isn't cluttered with resource de-allocation either in if bodies or in blocks reached by gotos. Jeff
Re: [RFC PATCH] add auto_bitmap
Jeff Law l...@redhat.com wrote:
On 11/14/13 04:04, tsaund...@mozilla.com wrote:
From: Trevor Saunders tsaund...@mozilla.com
Hi,
this patch adds and starts to use a class auto_bitmap, which is a
very thin
wrapper around bitmap. Its advantage is that it takes care of
delocation
automatically. So you can do things like
int
f ()
{
auto_bitmap x;
// do stuff with x
}
Another advantage of this class is it puts the bitmap_head struct on
the stack
instead of mallocing it or using a obstack.
I Think the biggest question is if I should make auto_bitmap a full
c++ified
wrapper around bitmap or if I should contiune just taking the
address of it
and passing it as a bitmap, but other comments are of course welcome
too.
I'd prefer to see it fully c++ified.
In response to one of Richi's comments, I spot checked the patch and
only found two occurrences where this lengthened the lifetime of the
bitmap in any significant way. The vast majority of the time any
increase in length was trivial.
Those instances are in tree-ssa-loop-ivopts.c and the other in
tree-ssa-strlen.c. I don't think you need to change anything for them,
I'm just pointed out that of all the stuff you changed, these were the
only ones I saw where lifetimes were changed significantly.
I still ask why we need a new type and cannot put this functionality into
bitmap_head itself.
Richard.
jeff
Re: [RFC PATCH] add auto_bitmap
On 11/14/13 14:14, Richard Biener wrote: I'm just pointed out that of all the stuff you changed, these were the only ones I saw where lifetimes were changed significantly. I still ask why we need a new type and cannot put this functionality into bitmap_head itself. Given that bitmap is just a *bitmap_head_def aren't we suggesting the same thing? jeff
Re: [RFC PATCH] add auto_bitmap
On Thu, Nov 14, 2013 at 02:33:00PM -0700, Jeff Law wrote: On 11/14/13 14:14, Richard Biener wrote: I'm just pointed out that of all the stuff you changed, these were the only ones I saw where lifetimes were changed significantly. I still ask why we need a new type and cannot put this functionality into bitmap_head itself. Given that bitmap is just a *bitmap_head_def aren't we suggesting the same thing? I think bitmap_head some_bitmap; is a little funny name wise, but auto_bitmap some_bitmap; is kind of funny too, so I think just having people use bitmap_head is fine if that's what people prefer. Its unfortunate bitmap itself isn't available, but I don't have a plan for dealing with bitmaps allocated in gc memory right now, so I guess they need to stay as they are. Trev jeff
