* [RFA][PATCH] Adding missing calls to bitmap_clear @ 2016-03-21 19:10 Jeff Law 2016-03-21 19:39 ` Bernd Schmidt 0 siblings, 1 reply; 6+ messages in thread From: Jeff Law @ 2016-03-21 19:10 UTC (permalink / raw) To: gcc-patches@gcc.gnu.org >> gcc-patches [-- 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; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA][PATCH] Adding missing calls to bitmap_clear 2016-03-21 19:10 [RFA][PATCH] Adding missing calls to bitmap_clear Jeff Law @ 2016-03-21 19:39 ` Bernd Schmidt 2016-03-21 20:40 ` Jeff Law 0 siblings, 1 reply; 6+ messages in thread From: Bernd Schmidt @ 2016-03-21 19:39 UTC (permalink / raw) To: Jeff Law, gcc-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA][PATCH] Adding missing calls to bitmap_clear 2016-03-21 19:39 ` Bernd Schmidt @ 2016-03-21 20:40 ` Jeff Law 2016-03-22 10:22 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Jeff Law @ 2016-03-21 20:40 UTC (permalink / raw) To: Bernd Schmidt, gcc-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA][PATCH] Adding missing calls to bitmap_clear 2016-03-21 20:40 ` Jeff Law @ 2016-03-22 10:22 ` Richard Biener 2016-04-28 19:35 ` Jeff Law 0 siblings, 1 reply; 6+ messages in thread From: Richard Biener @ 2016-03-22 10:22 UTC (permalink / raw) To: Jeff Law; +Cc: Bernd Schmidt, GCC Patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA][PATCH] Adding missing calls to bitmap_clear 2016-03-22 10:22 ` Richard Biener @ 2016-04-28 19:35 ` Jeff Law 2016-04-29 8:06 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Jeff Law @ 2016-04-28 19:35 UTC (permalink / raw) To: Richard Biener; +Cc: Bernd Schmidt, GCC Patches 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 (¬_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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA][PATCH] Adding missing calls to bitmap_clear 2016-04-28 19:35 ` Jeff Law @ 2016-04-29 8:06 ` Richard Biener 0 siblings, 0 replies; 6+ messages in thread From: Richard Biener @ 2016-04-29 8:06 UTC (permalink / raw) To: Jeff Law; +Cc: Bernd Schmidt, GCC Patches 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 (¬_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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-29 8:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-21 19:10 [RFA][PATCH] Adding missing calls to bitmap_clear Jeff Law 2016-03-21 19:39 ` Bernd Schmidt 2016-03-21 20:40 ` Jeff Law 2016-03-22 10:22 ` Richard Biener 2016-04-28 19:35 ` Jeff Law 2016-04-29 8:06 ` 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).