On 6/20/24 4:36 PM, Richard Biener wrote:
Am 20.06.2024 um 16:05 schrieb Andrew MacLeod <amacl...@redhat.com>:
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.
The global obstack is special, it’s init keeps a reference count. So yes, a
local obstack is cleaner.
Ok, since a local obstack is cleaner and a global one has the potential
to introduce subtle bugs, I have rebased the patch against current
mainline and will commit the attached if it passes tests.
Thanks for everyone's feedback.
Aldy
From cd7b03ba43a74ae808a3005ff0e66cd8fabdaea3 Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <al...@redhat.com>
Date: Wed, 19 Jun 2024 11:42:16 +0200
Subject: [PATCH] Avoid global bitmap space in ranger.
gcc/ChangeLog:
* gimple-range-cache.cc (update_list::update_list): Add m_bitmaps.
(update_list::~update_list): Initialize m_bitmaps.
* gimple-range-cache.h (ssa_lazy_cache): Add m_bitmaps.
* gimple-range.cc (enable_ranger): Remove global bitmap
initialization.
(disable_ranger): Remove global bitmap release.
---
gcc/gimple-range-cache.cc | 6 ++++--
gcc/gimple-range-cache.h | 9 +++++++--
gcc/gimple-range.cc | 4 ----
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/gcc/gimple-range-cache.cc b/gcc/gimple-range-cache.cc
index d84fd1ca0e8..6979a14cbaa 100644
--- a/gcc/gimple-range-cache.cc
+++ b/gcc/gimple-range-cache.cc
@@ -906,6 +906,7 @@ private:
vec<int> m_update_list;
int m_update_head;
bitmap m_propfail;
+ bitmap_obstack m_bitmaps;
};
// Create an update list.
@@ -915,7 +916,8 @@ update_list::update_list ()
m_update_list.create (0);
m_update_list.safe_grow_cleared (last_basic_block_for_fn (cfun) + 64);
m_update_head = -1;
- m_propfail = BITMAP_ALLOC (NULL);
+ bitmap_obstack_initialize (&m_bitmaps);
+ m_propfail = BITMAP_ALLOC (&m_bitmaps);
}
// Destroy an update list.
@@ -923,7 +925,7 @@ update_list::update_list ()
update_list::~update_list ()
{
m_update_list.release ();
- BITMAP_FREE (m_propfail);
+ bitmap_obstack_release (&m_bitmaps);
}
// Add BB to the list of blocks to update, unless it's already in the list.
diff --git a/gcc/gimple-range-cache.h b/gcc/gimple-range-cache.h
index 63410d5437e..0ea34d3f686 100644
--- a/gcc/gimple-range-cache.h
+++ b/gcc/gimple-range-cache.h
@@ -78,8 +78,12 @@ protected:
class ssa_lazy_cache : public ssa_cache
{
public:
- inline ssa_lazy_cache () { active_p = BITMAP_ALLOC (NULL); }
- inline ~ssa_lazy_cache () { BITMAP_FREE (active_p); }
+ inline ssa_lazy_cache ()
+ {
+ bitmap_obstack_initialize (&m_bitmaps);
+ active_p = BITMAP_ALLOC (&m_bitmaps);
+ }
+ inline ~ssa_lazy_cache () { bitmap_obstack_release (&m_bitmaps); }
inline bool empty_p () const { return bitmap_empty_p (active_p); }
virtual bool has_range (tree name) const;
virtual bool set_range (tree name, const vrange &r);
@@ -89,6 +93,7 @@ public:
virtual void clear ();
void merge (const ssa_lazy_cache &);
protected:
+ bitmap_obstack m_bitmaps;
bitmap active_p;
};
diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 50448ef81a2..5df649e268c 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -681,8 +681,6 @@ 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;
@@ -699,8 +697,6 @@ 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);
}
// ------------------------------------------------------------------------
--
2.45.0
