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?
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
> >
>
From aad99849a8ceb41cc8b8e81104b85736ac04a65a 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/gimple-range-cache.cc | 6 ++++--
gcc/gimple-range-cache.h | 9 +++++++--
gcc/gimple-range.cc | 9 +++------
gcc/gimple-range.h | 1 +
4 files changed, 15 insertions(+), 10 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 f3e4ec2d249..1c35f5a4e95 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -701,8 +701,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;
@@ -719,8 +717,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);
}
// ------------------------------------------------------------------------
@@ -930,7 +926,8 @@ dom_ranger::dom_ranger () : m_global ()
m_e0.safe_grow_cleared (last_basic_block_for_fn (cfun));
m_e1.create (0);
m_e1.safe_grow_cleared (last_basic_block_for_fn (cfun));
- m_pop_list = BITMAP_ALLOC (NULL);
+ bitmap_obstack_initialize (&m_bitmaps);
+ m_pop_list = BITMAP_ALLOC (&m_bitmaps);
if (dump_file && (param_ranger_debug & RANGER_DEBUG_TRACE))
tracer.enable_trace ();
}
@@ -945,7 +942,7 @@ dom_ranger::~dom_ranger ()
fprintf (dump_file, "=========================:\n");
m_global.dump (dump_file);
}
- BITMAP_FREE (m_pop_list);
+ bitmap_obstack_release (&m_bitmaps);
m_e1.release ();
m_e0.release ();
m_freelist.release ();
diff --git a/gcc/gimple-range.h b/gcc/gimple-range.h
index 180090bed15..264413ee458 100644
--- a/gcc/gimple-range.h
+++ b/gcc/gimple-range.h
@@ -124,6 +124,7 @@ protected:
vec<ssa_lazy_cache *> m_freelist;
vec<ssa_lazy_cache *> m_e0;
vec<ssa_lazy_cache *> m_e1;
+ bitmap_obstack m_bitmaps;
bitmap m_pop_list;
range_tracer tracer;
};
--
2.45.0
