* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack [not found] <20240408095018.4853D385841E@sourceware.org> @ 2024-04-08 15:40 ` Aldy Hernandez 2024-04-08 15:54 ` Jakub Jelinek 0 siblings, 1 reply; 14+ messages in thread From: Aldy Hernandez @ 2024-04-08 15:40 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek, amacleod On Mon, Apr 8, 2024 at 11:50 AM Richard Biener <rguenther@suse.de> wrote: > > The following fixes ranger bitmap allocation when invoked from IPA > context where the global bitmap obstack possibly isn't initialized. > Instead of trying to use one of the ranger obstacks the following > simply initializes the global bitmap obstack around an active ranger. > > Bootstrapped and tested on x86_64-unknown-linux-gnu with bitmap_alloc > instrumentation (all ICEs gone, so ranger was the only offender). > > OK? > > Thanks, > Richard. > > 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? If so, wouldn't it be better to lazily initialize it downstream (bitmap_alloc, or whomever needs it initialized)? Unless I'm misunderstanding something, it doesn't seem like ranger is the correct place to fix this. Aldy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-04-08 15:40 ` [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack Aldy Hernandez @ 2024-04-08 15:54 ` Jakub Jelinek 2024-04-08 16:08 ` Aldy Hernandez 0 siblings, 1 reply; 14+ messages in thread From: Jakub Jelinek @ 2024-04-08 15:54 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Richard Biener, gcc-patches, amacleod 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. If one uses this from a pass which has its own bitmap_obstack_{initializer,release} (NULL) around this, then the above will not do anything, just bump the nesting counter and decrease it later. But if some pass doesn't, i.e. the pass itself doesn't use the default obstack, only ranger does, then the above seems like the right change to me. > Unless I'm misunderstanding something, it > doesn't seem like ranger is the correct place to fix this. Jakub ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-04-08 15:54 ` Jakub Jelinek @ 2024-04-08 16:08 ` Aldy Hernandez 2024-04-08 16:28 ` Richard Biener 0 siblings, 1 reply; 14+ messages in thread From: Aldy Hernandez @ 2024-04-08 16:08 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Biener, gcc-patches, amacleod On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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. Aldy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-04-08 16:08 ` Aldy Hernandez @ 2024-04-08 16:28 ` Richard Biener 2024-04-08 16:39 ` Aldy Hernandez 0 siblings, 1 reply; 14+ messages in thread From: Richard Biener @ 2024-04-08 16:28 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Jakub Jelinek, gcc-patches, amacleod > Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: > > On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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 Richard > Aldy > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-04-08 16:28 ` Richard Biener @ 2024-04-08 16:39 ` Aldy Hernandez 2024-04-08 17:47 ` Richard Biener 0 siblings, 1 reply; 14+ messages in thread From: Aldy Hernandez @ 2024-04-08 16:39 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, gcc-patches, amacleod On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote: > > > > > Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: > > > > On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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. 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-04-08 16:39 ` Aldy Hernandez @ 2024-04-08 17:47 ` Richard Biener 2024-04-09 5:43 ` Aldy Hernandez 2024-06-20 8:08 ` Aldy Hernandez 0 siblings, 2 replies; 14+ messages in thread From: Richard Biener @ 2024-04-08 17:47 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Richard Biener, Jakub Jelinek, gcc-patches, amacleod > Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>: > > On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote: >> >> >> >>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: >>> >>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-04-08 17:47 ` Richard Biener @ 2024-04-09 5:43 ` Aldy Hernandez 2024-04-09 6:48 ` Richard Biener 2024-06-20 8:08 ` Aldy Hernandez 1 sibling, 1 reply; 14+ messages in thread From: Aldy Hernandez @ 2024-04-09 5:43 UTC (permalink / raw) To: Richard Biener; +Cc: Richard Biener, Jakub Jelinek, gcc-patches, amacleod 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. Aldy On Mon, Apr 8, 2024 at 7:47 PM Richard Biener <richard.guenther@gmail.com> wrote: > > > > > Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>: > > > > On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote: > >> > >> > >> > >>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: > >>> > >>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-04-09 5:43 ` Aldy Hernandez @ 2024-04-09 6:48 ` Richard Biener 0 siblings, 0 replies; 14+ messages in thread From: Richard Biener @ 2024-04-09 6:48 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Richard Biener, Jakub Jelinek, gcc-patches, amacleod [-- Attachment #1: Type: text/plain, Size: 5180 bytes --] 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 > <richard.guenther@gmail.com> wrote: > > > > > > > > > Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>: > > > > > > On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote: > > >> > > >> > > >> > > >>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: > > >>> > > >>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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 > > > > > > > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-04-08 17:47 ` Richard Biener 2024-04-09 5:43 ` Aldy Hernandez @ 2024-06-20 8:08 ` Aldy Hernandez 2024-06-20 9:31 ` Richard Biener 1 sibling, 1 reply; 14+ messages in thread From: Aldy Hernandez @ 2024-06-20 8:08 UTC (permalink / raw) To: Richard Biener; +Cc: Richard Biener, Jakub Jelinek, gcc-patches, amacleod [-- Attachment #1: Type: text/plain, Size: 4629 bytes --] 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.guenther@gmail.com> wrote: > > > > > Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>: > > > > On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote: > >> > >> > >> > >>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: > >>> > >>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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 > > > [-- Attachment #2: 0001-Avoid-global-bitmap-space-in-ranger.patch --] [-- Type: text/x-patch, Size: 3975 bytes --] From aad99849a8ceb41cc8b8e81104b85736ac04a65a Mon Sep 17 00:00:00 2001 From: Aldy Hernandez <aldyh@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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-06-20 8:08 ` Aldy Hernandez @ 2024-06-20 9:31 ` Richard Biener 2024-06-20 14:05 ` Andrew MacLeod 0 siblings, 1 reply; 14+ messages in thread From: Richard Biener @ 2024-06-20 9:31 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Richard Biener, Jakub Jelinek, gcc-patches, amacleod [-- Attachment #1: Type: text/plain, Size: 5258 bytes --] 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? I don't have a strong opinion. Richard. > Aldy > > On Mon, Apr 8, 2024 at 7:47 PM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > > > > > > Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>: > > > > > > On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote: > > >> > > >> > > >> > > >>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: > > >>> > > >>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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 > > > > > > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-06-20 9:31 ` Richard Biener @ 2024-06-20 14:05 ` Andrew MacLeod 2024-06-20 14:36 ` Richard Biener 0 siblings, 1 reply; 14+ messages in thread From: Andrew MacLeod @ 2024-06-20 14:05 UTC (permalink / raw) To: Richard Biener, Aldy Hernandez; +Cc: Richard Biener, Jakub Jelinek, gcc-patches 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.guenther@gmail.com> wrote: >>> >>> >>>> Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>: >>>> >>>> On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote: >>>>> >>>>> >>>>>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: >>>>>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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 >>>> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-06-20 14:05 ` Andrew MacLeod @ 2024-06-20 14:36 ` Richard Biener 2024-06-26 8:50 ` Aldy Hernandez 0 siblings, 1 reply; 14+ messages in thread From: Richard Biener @ 2024-06-20 14:36 UTC (permalink / raw) To: Andrew MacLeod; +Cc: Aldy Hernandez, Richard Biener, Jakub Jelinek, gcc-patches > Am 20.06.2024 um 16:05 schrieb Andrew MacLeod <amacleod@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.guenther@gmail.com> wrote: >>>> >>>> >>>>> Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>: >>>>> >>>>> On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote: >>>>>> >>>>>> >>>>>>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>: >>>>>>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@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 >>>>> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack 2024-06-20 14:36 ` Richard Biener @ 2024-06-26 8:50 ` Aldy Hernandez 0 siblings, 0 replies; 14+ messages in thread From: Aldy Hernandez @ 2024-06-26 8:50 UTC (permalink / raw) To: Richard Biener, Andrew MacLeod; +Cc: Richard Biener, Jakub Jelinek, gcc-patches [-- Attachment #1: Type: text/plain, Size: 2556 bytes --] On 6/20/24 4:36 PM, Richard Biener wrote: > > >> Am 20.06.2024 um 16:05 schrieb Andrew MacLeod <amacleod@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 [-- Attachment #2: 0001-Avoid-global-bitmap-space-in-ranger.patch --] [-- Type: text/x-patch, Size: 3201 bytes --] From cd7b03ba43a74ae808a3005ff0e66cd8fabdaea3 Mon Sep 17 00:00:00 2001 From: Aldy Hernandez <aldyh@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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack @ 2024-04-08 9:49 Richard Biener 0 siblings, 0 replies; 14+ messages in thread From: Richard Biener @ 2024-04-08 9:49 UTC (permalink / raw) To: gcc-patches; +Cc: Jakub Jelinek, amacleod The following fixes ranger bitmap allocation when invoked from IPA context where the global bitmap obstack possibly isn't initialized. Instead of trying to use one of the ranger obstacks the following simply initializes the global bitmap obstack around an active ranger. Bootstrapped and tested on x86_64-unknown-linux-gnu with bitmap_alloc instrumentation (all ICEs gone, so ranger was the only offender). OK? Thanks, Richard. 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); } // ------------------------------------------------------------------------ -- 2.35.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-06-26 8:50 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20240408095018.4853D385841E@sourceware.org>
2024-04-08 15:40 ` [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack Aldy Hernandez
2024-04-08 15:54 ` Jakub Jelinek
2024-04-08 16:08 ` Aldy Hernandez
2024-04-08 16:28 ` Richard Biener
2024-04-08 16:39 ` Aldy Hernandez
2024-04-08 17:47 ` Richard Biener
2024-04-09 5:43 ` Aldy Hernandez
2024-04-09 6:48 ` Richard Biener
2024-06-20 8:08 ` Aldy Hernandez
2024-06-20 9:31 ` Richard Biener
2024-06-20 14:05 ` Andrew MacLeod
2024-06-20 14:36 ` Richard Biener
2024-06-26 8:50 ` Aldy Hernandez
2024-04-08 9:49 Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).