[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
>
>