> 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.
Richard
>> 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
>>>>>
>
