[RFA][PATCH] Adding missing calls to bitmap_clear

[RFA][PATCH] Adding missing calls to bitmap_clear

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 414 bytes --]


As noted last week, find_removable_extensions initializes several 
bitmaps, but doesn't clear them.

This is not strictly a leak as the GC system should find dead data, but 
it's better to go ahead and clear the bitmaps.  That releases the 
elements back to the cache and presumably makes things easier for the GC 
system as well.

Bootstrapped and regression tested on x86_64-linux-gnu.

OK for the trunk?

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 375 bytes --]

	* ree.c (find_removable_extensions): Clear the local bitmaps.

diff --git a/gcc/ree.c b/gcc/ree.c
index 13a7a05..3dc180c 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -1139,6 +1139,10 @@ find_removable_extensions (void)
     }
 
   XDELETEVEC (def_map);
+  bitmap_clear (&init);
+  bitmap_clear (&kill);
+  bitmap_clear (&gen);
+  bitmap_clear (&tmp);
 
   return insn_list;
 }

Re: [RFA][PATCH] Adding missing calls to bitmap_clear

[Bernd Schmidt]

On 03/21/2016 08:06 PM, Jeff Law wrote:
>
> As noted last week, find_removable_extensions initializes several
> bitmaps, but doesn't clear them.
>
> This is not strictly a leak as the GC system should find dead data, but
> it's better to go ahead and clear the bitmaps.  That releases the
> elements back to the cache and presumably makes things easier for the GC
> system as well.
>
> Bootstrapped and regression tested on x86_64-linux-gnu.
>
> OK for the trunk?

Looks like they don't leak anywhere, so ok. Probably ok even to install 
it now but maybe stage1 would be better timing.


Bernd

Re: [RFA][PATCH] Adding missing calls to bitmap_clear

[Jeff Law]

On 03/21/2016 01:10 PM, Bernd Schmidt wrote:
> On 03/21/2016 08:06 PM, Jeff Law wrote:
>>
>> As noted last week, find_removable_extensions initializes several
>> bitmaps, but doesn't clear them.
>>
>> This is not strictly a leak as the GC system should find dead data, but
>> it's better to go ahead and clear the bitmaps.  That releases the
>> elements back to the cache and presumably makes things easier for the GC
>> system as well.
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu.
>>
>> OK for the trunk?
>
> Looks like they don't leak anywhere, so ok. Probably ok even to install
> it now but maybe stage1 would be better timing.
I don't mind waiting for the next stage1, this is a pretty minor issue.

jeff

Re: [RFA][PATCH] Adding missing calls to bitmap_clear

[Richard Biener]

On Mon, Mar 21, 2016 at 9:32 PM, Jeff Law <law@redhat.com> wrote:
> On 03/21/2016 01:10 PM, Bernd Schmidt wrote:
>>
>> On 03/21/2016 08:06 PM, Jeff Law wrote:
>>>
>>>
>>> As noted last week, find_removable_extensions initializes several
>>> bitmaps, but doesn't clear them.
>>>
>>> This is not strictly a leak as the GC system should find dead data, but
>>> it's better to go ahead and clear the bitmaps.  That releases the
>>> elements back to the cache and presumably makes things easier for the GC
>>> system as well.
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu.
>>>
>>> OK for the trunk?
>>
>>
>> Looks like they don't leak anywhere, so ok. Probably ok even to install
>> it now but maybe stage1 would be better timing.
>
> I don't mind waiting for the next stage1, this is a pretty minor issue.

It's ok at this stage as it will also fix -fmem-report.  Please also move
the thing back to heap, see below.

Btw we should disallow bitmap_initialize (&x, NULL) as it does not do
the same thing as BITMAP_ALLOC (NULL), it does the same thing
as BITMAP_ALLOC_GC ().  Thus I'd rather have a bitmap_initialize_gc (&x)
and a bitmap_initialize (&x, NULL) that ends up using the global
bitmap obstack.  No idea where REE came from history wise.

A grep shows only

ira.c:  bitmap_initialize (&seen_insns, NULL);
ree.c:  bitmap_initialize (&init, NULL);
ree.c:  bitmap_initialize (&kill, NULL);
ree.c:  bitmap_initialize (&gen, NULL);
ree.c:  bitmap_initialize (&tmp, NULL);

btw, so please consider simply changing bitmap_initialize behavior.  The IRA
use also should use the global bitmap obstack as users around that use
use BITMAP_ALLOC (NULL).  [use a default arg for 'obstack' if possible,
you have to verify it works with/without --enable-gather-detailed-mem-stats]

Thanks,
Richard.

> jeff

Re: [RFA][PATCH] Adding missing calls to bitmap_clear

[Jeff Law]

On 03/22/2016 03:37 AM, Richard Biener wrote:
> On Mon, Mar 21, 2016 at 9:32 PM, Jeff Law <law@redhat.com> wrote:
>> On 03/21/2016 01:10 PM, Bernd Schmidt wrote:
>>>
>>> On 03/21/2016 08:06 PM, Jeff Law wrote:
>>>>
>>>>
>>>> As noted last week, find_removable_extensions initializes several
>>>> bitmaps, but doesn't clear them.
>>>>
>>>> This is not strictly a leak as the GC system should find dead data, but
>>>> it's better to go ahead and clear the bitmaps.  That releases the
>>>> elements back to the cache and presumably makes things easier for the GC
>>>> system as well.
>>>>
>>>> Bootstrapped and regression tested on x86_64-linux-gnu.
>>>>
>>>> OK for the trunk?
>>>
>>>
>>> Looks like they don't leak anywhere, so ok. Probably ok even to install
>>> it now but maybe stage1 would be better timing.
>>
>> I don't mind waiting for the next stage1, this is a pretty minor issue.
>
> It's ok at this stage as it will also fix -fmem-report.  Please also move
> the thing back to heap, see below.
>
> Btw we should disallow bitmap_initialize (&x, NULL) as it does not do
> the same thing as BITMAP_ALLOC (NULL), it does the same thing
> as BITMAP_ALLOC_GC ().  Thus I'd rather have a bitmap_initialize_gc (&x)
> and a bitmap_initialize (&x, NULL) that ends up using the global
> bitmap obstack.  No idea where REE came from history wise.
>
> A grep shows only
>
> ira.c:  bitmap_initialize (&seen_insns, NULL);
> ree.c:  bitmap_initialize (&init, NULL);
> ree.c:  bitmap_initialize (&kill, NULL);
> ree.c:  bitmap_initialize (&gen, NULL);
> ree.c:  bitmap_initialize (&tmp, NULL);
It's more than that.  Sadly folks have passed in "0" instead of NULL in 
various places.

./haifa-sched.c:  bitmap_initialize (&processed, 0);
./haifa-sched.c:  bitmap_initialize (&processed, 0);
./haifa-sched.c:  bitmap_initialize (&in_ready, 0);
./sched-ebb.c:  bitmap_initialize (&dont_calc_deps, 0);
./sched-rgn.c:  bitmap_initialize (&not_in_df, 0);
./testsuite/gcc.dg/pr45352.c:      bitmap_initialize_stat (0);
./ira.c:  bitmap_initialize (&interesting, 0);
./ira.c:  bitmap_initialize (&live, 0);
./ira.c:  bitmap_initialize (&used, 0);
./ira.c:  bitmap_initialize (&set, 0);
./ira.c:  bitmap_initialize (&unusable_as_input, 0);
./ira.c:      bitmap_initialize (local, 0);
./ira.c:      bitmap_initialize (transp, 0);
./ira.c:      bitmap_initialize (moveable, 0);
./ira.c:  bitmap_initialize (&need_new, 0);
./ira.c:  bitmap_initialize (&reachable, 0);
./sel-sched.c:  bitmap_initialize (forced_ebb_heads, 0);
./sched-deps.c:   bitmap_initialize (&true_dependency_cache[i], 0);
./sched-deps.c:   bitmap_initialize (&output_dependency_cache[i], 0);
./sched-deps.c:   bitmap_initialize (&anti_dependency_cache[i], 0);
./sched-deps.c:   bitmap_initialize (&control_dependency_cache[i], 0);
./sched-deps.c:            bitmap_initialize (&spec_dependency_cache[i], 0);

>
> btw, so please consider simply changing bitmap_initialize behavior.  The IRA
> use also should use the global bitmap obstack as users around that use
> use BITMAP_ALLOC (NULL).  [use a default arg for 'obstack' if possible,
> you have to verify it works with/without --enable-gather-detailed-mem-stats]
The problem is ensuring that allocating off the default bitmap obstack 
is appropriate for all those uses.

I'm tempted to change them all to NULL.  Then iterate one by one on to 
ensure we're routing to gc vs the default bitmap obstack as appropriate 
and that we're calling bitmap_clear as appropriate.

Once we've fixed all of 'em, we simply assert that bitmap_initialize is 
never passed NULL and avoid getting in this situation again in the future.

Thoughts?
jeff

Re: [RFA][PATCH] Adding missing calls to bitmap_clear

[Richard Biener]

On Thu, Apr 28, 2016 at 9:35 PM, Jeff Law <law@redhat.com> wrote:
> On 03/22/2016 03:37 AM, Richard Biener wrote:
>>
>> On Mon, Mar 21, 2016 at 9:32 PM, Jeff Law <law@redhat.com> wrote:
>>>
>>> On 03/21/2016 01:10 PM, Bernd Schmidt wrote:
>>>>
>>>>
>>>> On 03/21/2016 08:06 PM, Jeff Law wrote:
>>>>>
>>>>>
>>>>>
>>>>> As noted last week, find_removable_extensions initializes several
>>>>> bitmaps, but doesn't clear them.
>>>>>
>>>>> This is not strictly a leak as the GC system should find dead data, but
>>>>> it's better to go ahead and clear the bitmaps.  That releases the
>>>>> elements back to the cache and presumably makes things easier for the
>>>>> GC
>>>>> system as well.
>>>>>
>>>>> Bootstrapped and regression tested on x86_64-linux-gnu.
>>>>>
>>>>> OK for the trunk?
>>>>
>>>>
>>>>
>>>> Looks like they don't leak anywhere, so ok. Probably ok even to install
>>>> it now but maybe stage1 would be better timing.
>>>
>>>
>>> I don't mind waiting for the next stage1, this is a pretty minor issue.
>>
>>
>> It's ok at this stage as it will also fix -fmem-report.  Please also move
>> the thing back to heap, see below.
>>
>> Btw we should disallow bitmap_initialize (&x, NULL) as it does not do
>> the same thing as BITMAP_ALLOC (NULL), it does the same thing
>> as BITMAP_ALLOC_GC ().  Thus I'd rather have a bitmap_initialize_gc (&x)
>> and a bitmap_initialize (&x, NULL) that ends up using the global
>> bitmap obstack.  No idea where REE came from history wise.
>>
>> A grep shows only
>>
>> ira.c:  bitmap_initialize (&seen_insns, NULL);
>> ree.c:  bitmap_initialize (&init, NULL);
>> ree.c:  bitmap_initialize (&kill, NULL);
>> ree.c:  bitmap_initialize (&gen, NULL);
>> ree.c:  bitmap_initialize (&tmp, NULL);
>
> It's more than that.  Sadly folks have passed in "0" instead of NULL in
> various places.
>
> ./haifa-sched.c:  bitmap_initialize (&processed, 0);
> ./haifa-sched.c:  bitmap_initialize (&processed, 0);
> ./haifa-sched.c:  bitmap_initialize (&in_ready, 0);
> ./sched-ebb.c:  bitmap_initialize (&dont_calc_deps, 0);
> ./sched-rgn.c:  bitmap_initialize (&not_in_df, 0);
> ./testsuite/gcc.dg/pr45352.c:      bitmap_initialize_stat (0);
> ./ira.c:  bitmap_initialize (&interesting, 0);
> ./ira.c:  bitmap_initialize (&live, 0);
> ./ira.c:  bitmap_initialize (&used, 0);
> ./ira.c:  bitmap_initialize (&set, 0);
> ./ira.c:  bitmap_initialize (&unusable_as_input, 0);
> ./ira.c:      bitmap_initialize (local, 0);
> ./ira.c:      bitmap_initialize (transp, 0);
> ./ira.c:      bitmap_initialize (moveable, 0);
> ./ira.c:  bitmap_initialize (&need_new, 0);
> ./ira.c:  bitmap_initialize (&reachable, 0);
> ./sel-sched.c:  bitmap_initialize (forced_ebb_heads, 0);
> ./sched-deps.c:   bitmap_initialize (&true_dependency_cache[i], 0);
> ./sched-deps.c:   bitmap_initialize (&output_dependency_cache[i], 0);
> ./sched-deps.c:   bitmap_initialize (&anti_dependency_cache[i], 0);
> ./sched-deps.c:   bitmap_initialize (&control_dependency_cache[i], 0);
> ./sched-deps.c:            bitmap_initialize (&spec_dependency_cache[i], 0);
>
>>
>> btw, so please consider simply changing bitmap_initialize behavior.  The
>> IRA
>> use also should use the global bitmap obstack as users around that use
>> use BITMAP_ALLOC (NULL).  [use a default arg for 'obstack' if possible,
>> you have to verify it works with/without
>> --enable-gather-detailed-mem-stats]
>
> The problem is ensuring that allocating off the default bitmap obstack is
> appropriate for all those uses.

True, if the bitmap head lives in a GC structure then that's not safe.

> I'm tempted to change them all to NULL.  Then iterate one by one on to
> ensure we're routing to gc vs the default bitmap obstack as appropriate and
> that we're calling bitmap_clear as appropriate.
>
> Once we've fixed all of 'em, we simply assert that bitmap_initialize is
> never passed NULL and avoid getting in this situation again in the future.

First one sounds good.  I'd still add a bitmap_gc_initialize (&head) and change
bitmap_initialize (&head, NULL) behavior to match that of BITMAP_ALLOC (NULL).

Richard.

> Thoughts?
> jeff
>
>