On Tue, 9 Apr 2024, Aldy Hernandez wrote: > BTW, I'm not opposed to this patch. Thank you for tracking this down, > and feel free to commit as is if y'all PMs agree it's OK. I just > wanted to know if there's a better way going forward. I can certainly > put it on my TODO list once stage1 opens again. > > And no, there probably isn't an obstack for those classes, but I > wonder if we should have a class local one, as we do for the rest of > the classes. OK, I pushed it now, it looks like the GCC 13 branch isn't affected in obviously the same way (but I didn't try instrumenting there). Feel free to improve next stage1. Richard. > Aldy > > On Mon, Apr 8, 2024 at 7:47 PM Richard Biener > wrote: > > > > > > > > > Am 08.04.2024 um 18:40 schrieb Aldy Hernandez : > > > > > > On Mon, Apr 8, 2024 at 6:29 PM Richard Biener wrote: > > >> > > >> > > >> > > >>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez : > > >>> > > >>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek 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 > > > > > > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)