On 6/20/24 05:31, Richard Biener wrote:
On Thu, 20 Jun 2024, Aldy Hernandez wrote:
Hi.
I came around to this, and whipped up the proposed patch. However, it
does seem a bit verbose, and I'm wondering if it's cleaner to just
leave things as they are.
The attached patch passes tests and there's no difference in
performance. I am wondering, whether it's better to get rid of
all/most of the local obstacks we use in ranger, and just use the
global (NULL) one?
Thoughts?
It really depends on how much garbage ranger is expected to create
on the obstack - the global obstack is released after each pass.
But ranger instances are also not expected to be created multiple
times each pass, right?
Typically correct. Although the path ranger also creates a normal
ranger,. Different components also have their own obstacks, mostly
because they can be used independent of ranger. I didn't want to add
artificial dependencies just for a obstack sharing.
I was unaware of how the global one worked at that point. Do they get
stacked if another global obstack is initialized? And is there any
danger in that case twe could accidentally have a sequence like:
obstack1 created by ranger
GORI allocates bitmap from obstack1
obstack2 created by the pass that decided to use ranger
GORI allocates bitmap2.. comes from obstack2
obstack2 destroyed by the pass.
GORI tries to use bitmap2 .. its now unallocated.
If so, this reeks of the obstack problems we had back in the late 90's
when obstacks were generally stacked. Tracking down objects still in
use from freed obstacks was a nightmare. That is one of the reason
general stacked obstacks fell out of favour for a long time, and why i
only ever use local named one.
It seems to me that components managing their own obstacks ensures
this does not happen.
If, however, that is not longer a problem for some reason, then I have
no strong feelings either way either.
I don't have a strong opinion.
Richard.
Aldy
On Mon, Apr 8, 2024 at 7:47 PM Richard Biener
<richard.guent...@gmail.com> wrote:
Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <al...@redhat.com>:
On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguent...@suse.de> wrote:
Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <al...@redhat.com>:
On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <ja...@redhat.com> wrote:
On Mon, Apr 08, 2024 at 05:40:23PM +0200, Aldy Hernandez wrote:
PR middle-end/114604
* gimple-range.cc (enable_ranger): Initialize the global
bitmap obstack.
(disable_ranger): Release it.
---
gcc/gimple-range.cc | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index c16b776c1e3..4d3b1ce8588 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
{
gimple_ranger *r;
+ bitmap_obstack_initialize (NULL);
+
gcc_checking_assert (!fun->x_range_query);
r = new gimple_ranger (use_imm_uses);
fun->x_range_query = r;
@@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
gcc_checking_assert (fun->x_range_query);
delete fun->x_range_query;
fun->x_range_query = NULL;
+
+ bitmap_obstack_release (NULL);
Are you not allowed to initialize/use obstacks unless
bitmap_obstack_initialize(NULL) is called?
You can use it with some other obstack, just not the default one.
If so, wouldn't it be
better to lazily initialize it downstream (bitmap_alloc, or whomever
needs it initialized)?
No, you still need to decide where is the safe point to release it.
Unlike the non-default bitmap_obstack_initialize/bitmap_obstack_release,
the default one can nest (has associated nesting counter). So, the above
patch just says that ranger starts using the default obstack in
enable_ranger and stops using it in disable_ranger and anything ranger
associated in the obstack can be freed at that point.
I thought ranger never used the default one:
$ grep bitmap_obstack_initialize *value* *range*
value-relation.cc: bitmap_obstack_initialize (&m_bitmaps);
value-relation.cc: bitmap_obstack_initialize (&m_bitmaps);
gimple-range-cache.cc: bitmap_obstack_initialize (&m_bitmaps);
gimple-range-gori.cc: bitmap_obstack_initialize (&m_bitmaps);
gimple-range-infer.cc: bitmap_obstack_initialize (&m_bitmaps);
gimple-range-phi.cc: bitmap_obstack_initialize (&m_bitmaps);
or even:
$ grep obstack.*NULL *value* *range*
value-range-storage.cc: obstack_free (&m_obstack, NULL);
value-relation.cc: obstack_free (&m_chain_obstack, NULL);
value-relation.cc: obstack_free (&m_chain_obstack, NULL);
gimple-range-infer.cc: obstack_free (&m_list_obstack, NULL);
value-range-storage.cc: obstack_free (&m_obstack, NULL);
I'm obviously missing something here.
Look for BITMAP_ALLOC (NULL) in the backtrace in the PR
Ahh! Thanks.
A few default obstack uses snuck in while I wasn't looking.
$ grep BITMAP_ALLOC.*NULL *range*
gimple-range-cache.cc: m_propfail = BITMAP_ALLOC (NULL);
gimple-range-cache.h: inline ssa_lazy_cache () { active_p =
BITMAP_ALLOC (NULL); }
gimple-range.cc: m_pop_list = BITMAP_ALLOC (NULL);
I wonder if it would be cleaner to just change these to use named obstacks.
I didn’t find any obvious obstack to use, but sure. This was the easiest fix ;)
Richard
Andrew, is there a reason we were using the default obstack for these?
For reference, they are class update_list used in the ranger cache,
ssa_lazy_cache, and dom_ranger.
Aldy
