* Do not leak memory when streaming in ssa names @ 2020-11-23 12:18 Jan Hubicka 2020-11-23 12:27 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2020-11-23 12:18 UTC (permalink / raw) To: gcc-patches, rguenther, mliska Hi, tree-streamer-in currently calls init_tree_ssa that calls init_ssanames that allocates space in ssa_names array for 50 names. Later it streams in the count and calls init_tree_ssa again, this time with correct count, which is rounded up to 50 and allocated again leaking the first array. This patch fixes it by adding extra argument to init_tree_ssa specifing whether ssa names should be initialized. There are few places where we initialize ssa and only lto streaming is special, so i think extra arg works well here. I am not quite sure about the 50MB default. I think it was made before ggc_free was implemented (so resizing vectors was expensive) and when we had only one function in SSA form at a time. Most of functions in C++ will probably need fewer than 50 SSA names until inlining. This saves 21MB of WPA linking libxul. Bootstrapping/regtesting x86_64-linux, OK if it suceeds? * tree-ssa.c (init_tree_ssa): Add argument INIT_SSANAMES_P. * tree-ssa.h (init_tree_ssa): Update prototype. * tree-ssanames.c (init_ssanames): Fix handling of explicit size. * tree-streamer-in.c (init_function): Pass false to init_tree_ssa. diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c index b44361f8244..8b009d91830 100644 --- a/gcc/tree-ssa.c +++ b/gcc/tree-ssa.c @@ -1218,15 +1218,18 @@ err: # pragma GCC diagnostic pop #endif -/* Initialize global DFA and SSA structures. */ +/* Initialize global DFA and SSA structures. + If INIT_SSANAMES_P is false, do not initialize SSA names. During LTO + streaming we do that later once we know number of names. */ void -init_tree_ssa (struct function *fn) +init_tree_ssa (struct function *fn, bool init_ssanames_p) { fn->gimple_df = ggc_cleared_alloc<gimple_df> (); fn->gimple_df->default_defs = hash_table<ssa_name_hasher>::create_ggc (20); pt_solution_reset (&fn->gimple_df->escaped); - init_ssanames (fn, 0); + if (init_ssanames_p) + init_ssanames (fn, 0); } /* Deallocate memory associated with SSA data structures for FNDECL. */ diff --git a/gcc/tree-ssa.h b/gcc/tree-ssa.h index 79f2b13fd6a..9a4ff008300 100644 --- a/gcc/tree-ssa.h +++ b/gcc/tree-ssa.h @@ -45,7 +45,7 @@ extern void insert_debug_temps_for_defs (gimple_stmt_iterator *); extern void reset_debug_uses (gimple *); extern void release_defs_bitset (bitmap toremove); extern void verify_ssa (bool, bool); -extern void init_tree_ssa (function *); +extern void init_tree_ssa (function *, bool init_ssanames = true); extern void delete_tree_ssa (function *); extern bool tree_ssa_useless_type_conversion (tree); extern tree tree_ssa_strip_useless_type_conversions (tree); diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c index 6ac97fe39c4..ec4681f85cb 100644 --- a/gcc/tree-ssanames.c +++ b/gcc/tree-ssanames.c @@ -77,10 +77,10 @@ unsigned int ssa_name_nodes_created; void init_ssanames (struct function *fn, int size) { - if (size < 50) - size = 50; - - vec_alloc (SSANAMES (fn), size); + if (!size) + vec_alloc (SSANAMES (fn), 50); + else + vec_safe_reserve (SSANAMES (fn), size, true); /* Version 0 is special, so reserve the first slot in the table. Though currently unused, we may use version 0 in alias analysis as part of diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c index 64037f74ad3..3150c489401 100644 --- a/gcc/lto-streamer-in.c +++ b/gcc/lto-streamer-in.c @@ -1384,7 +1384,7 @@ input_function (tree fn_decl, class data_in *data_in, push_struct_function (fn_decl); fn = DECL_STRUCT_FUNCTION (fn_decl); - init_tree_ssa (fn); + init_tree_ssa (fn, false); /* We input IL in SSA form. */ cfun->gimple_df->in_ssa_p = true; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do not leak memory when streaming in ssa names 2020-11-23 12:18 Do not leak memory when streaming in ssa names Jan Hubicka @ 2020-11-23 12:27 ` Richard Biener 2020-11-23 12:43 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2020-11-23 12:27 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, mliska On Mon, 23 Nov 2020, Jan Hubicka wrote: > Hi, > tree-streamer-in currently calls init_tree_ssa that calls init_ssanames > that allocates space in ssa_names array for 50 names. Later it streams > in the count and calls init_tree_ssa again, this time with correct > count, which is rounded up to 50 and allocated again leaking the first > array. > > This patch fixes it by adding extra argument to init_tree_ssa specifing > whether ssa names should be initialized. There are few places where we > initialize ssa and only lto streaming is special, so i think extra arg > works well here. > > I am not quite sure about the 50MB default. I think it was made before > ggc_free was implemented (so resizing vectors was expensive) and when we > had only one function in SSA form at a time. Most of functions in C++ > will probably need fewer than 50 SSA names until inlining. > > This saves 21MB of WPA linking libxul. Why do we input SSA names at WPA? > Bootstrapping/regtesting x86_64-linux, OK if it suceeds? I think calling init_ssanames again is bogus. Please simply do that in input_ssa_names after we know how many we need adding a number of SSA names to reserve argument to init_tree_ssa and passing that down instead. Richard. > * tree-ssa.c (init_tree_ssa): Add argument INIT_SSANAMES_P. > * tree-ssa.h (init_tree_ssa): Update prototype. > * tree-ssanames.c (init_ssanames): Fix handling of explicit size. > * tree-streamer-in.c (init_function): Pass false to init_tree_ssa. > diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c > index b44361f8244..8b009d91830 100644 > --- a/gcc/tree-ssa.c > +++ b/gcc/tree-ssa.c > @@ -1218,15 +1218,18 @@ err: > # pragma GCC diagnostic pop > #endif > > -/* Initialize global DFA and SSA structures. */ > +/* Initialize global DFA and SSA structures. > + If INIT_SSANAMES_P is false, do not initialize SSA names. During LTO > + streaming we do that later once we know number of names. */ > > void > -init_tree_ssa (struct function *fn) > +init_tree_ssa (struct function *fn, bool init_ssanames_p) > { > fn->gimple_df = ggc_cleared_alloc<gimple_df> (); > fn->gimple_df->default_defs = hash_table<ssa_name_hasher>::create_ggc (20); > pt_solution_reset (&fn->gimple_df->escaped); > - init_ssanames (fn, 0); > + if (init_ssanames_p) > + init_ssanames (fn, 0); > } > > /* Deallocate memory associated with SSA data structures for FNDECL. */ > diff --git a/gcc/tree-ssa.h b/gcc/tree-ssa.h > index 79f2b13fd6a..9a4ff008300 100644 > --- a/gcc/tree-ssa.h > +++ b/gcc/tree-ssa.h > @@ -45,7 +45,7 @@ extern void insert_debug_temps_for_defs (gimple_stmt_iterator *); > extern void reset_debug_uses (gimple *); > extern void release_defs_bitset (bitmap toremove); > extern void verify_ssa (bool, bool); > -extern void init_tree_ssa (function *); > +extern void init_tree_ssa (function *, bool init_ssanames = true); > extern void delete_tree_ssa (function *); > extern bool tree_ssa_useless_type_conversion (tree); > extern tree tree_ssa_strip_useless_type_conversions (tree); > diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c > index 6ac97fe39c4..ec4681f85cb 100644 > --- a/gcc/tree-ssanames.c > +++ b/gcc/tree-ssanames.c > @@ -77,10 +77,10 @@ unsigned int ssa_name_nodes_created; > void > init_ssanames (struct function *fn, int size) > { > - if (size < 50) > - size = 50; > - > - vec_alloc (SSANAMES (fn), size); > + if (!size) > + vec_alloc (SSANAMES (fn), 50); > + else > + vec_safe_reserve (SSANAMES (fn), size, true); > > /* Version 0 is special, so reserve the first slot in the table. Though > currently unused, we may use version 0 in alias analysis as part of > diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c > index 64037f74ad3..3150c489401 100644 > --- a/gcc/lto-streamer-in.c > +++ b/gcc/lto-streamer-in.c > @@ -1384,7 +1384,7 @@ input_function (tree fn_decl, class data_in *data_in, > > push_struct_function (fn_decl); > fn = DECL_STRUCT_FUNCTION (fn_decl); > - init_tree_ssa (fn); > + init_tree_ssa (fn, false); > /* We input IL in SSA form. */ > cfun->gimple_df->in_ssa_p = true; > > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imend ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do not leak memory when streaming in ssa names 2020-11-23 12:27 ` Richard Biener @ 2020-11-23 12:43 ` Jan Hubicka 2020-11-23 12:49 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2020-11-23 12:43 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > Hi, > > tree-streamer-in currently calls init_tree_ssa that calls init_ssanames > > that allocates space in ssa_names array for 50 names. Later it streams > > in the count and calls init_tree_ssa again, this time with correct > > count, which is rounded up to 50 and allocated again leaking the first > > array. > > > > This patch fixes it by adding extra argument to init_tree_ssa specifing > > whether ssa names should be initialized. There are few places where we > > initialize ssa and only lto streaming is special, so i think extra arg > > works well here. > > > > I am not quite sure about the 50MB default. I think it was made before > > ggc_free was implemented (so resizing vectors was expensive) and when we > > had only one function in SSA form at a time. Most of functions in C++ > > will probably need fewer than 50 SSA names until inlining. > > > > This saves 21MB of WPA linking libxul. > > Why do we input SSA names at WPA? To ICF compare function bodies. It tends to load tons of small bodies. We also may load body when merging BB profile etc. > > > Bootstrapping/regtesting x86_64-linux, OK if it suceeds? > > I think calling init_ssanames again is bogus. Please simply > do that in input_ssa_names after we know how many we need > adding a number of SSA names to reserve argument to init_tree_ssa > and passing that down instead. I tried that originally but it hit ICE in init_ssa_operands. Seems that reloacating them is last change I need though. Does this look better? * tree-cfg.c (remove_bb): Release SSA defs. * tree-ssa.c (verify_flow_sensitive_alias_info): Do not walk dead nodes. * tree-tailcall.c (eliminate_tail_call): Release SSA name. Index: tree-cfg.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/tree-cfg.c,v retrieving revision 2.47 diff -c -3 -p -r2.47 tree-cfg.c *** tree-cfg.c 6 Sep 2004 10:07:56 -0000 2.47 --- tree-cfg.c 7 Sep 2004 13:26:46 -0000 *************** remove_bb (basic_block bb) *** 1818,1823 **** --- 1818,1824 ---- for (i = bsi_start (bb); !bsi_end_p (i); bsi_remove (&i)) { tree stmt = bsi_stmt (i); + release_defs (stmt); set_bb_for_stmt (stmt, NULL); Index: tree-tailcall.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/tree-tailcall.c,v retrieving revision 2.23 diff -c -3 -p -r2.23 tree-tailcall.c *** tree-tailcall.c 6 Sep 2004 10:08:13 -0000 2.23 --- tree-tailcall.c 7 Sep 2004 13:26:47 -0000 *************** eliminate_tail_call (struct tailcall *t) *** 681,692 **** bsi_next (&bsi); while (!bsi_end_p (bsi)) { /* Do not remove the return statement, so that redirect_edge_and_branch sees how the block ends. */ ! if (TREE_CODE (bsi_stmt (bsi)) == RETURN_EXPR) break; bsi_remove (&bsi); } /* Replace the call by a jump to the start of function. */ --- 681,694 ---- bsi_next (&bsi); while (!bsi_end_p (bsi)) { + tree t = bsi_stmt (bsi); /* Do not remove the return statement, so that redirect_edge_and_branch sees how the block ends. */ ! if (TREE_CODE (t) == RETURN_EXPR) break; bsi_remove (&bsi); + release_defs (t); } /* Replace the call by a jump to the start of function. */ *************** eliminate_tail_call (struct tailcall *t) *** 772,777 **** --- 774,780 ---- } bsi_remove (&t->call_bsi); + release_defs (call); } /* Optimizes the tailcall described by T. If OPT_TAILCALLS is true, also ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do not leak memory when streaming in ssa names 2020-11-23 12:43 ` Jan Hubicka @ 2020-11-23 12:49 ` Richard Biener 2020-11-23 12:56 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2020-11-23 12:49 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On Mon, 23 Nov 2020, Jan Hubicka wrote: > > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > > Hi, > > > tree-streamer-in currently calls init_tree_ssa that calls init_ssanames > > > that allocates space in ssa_names array for 50 names. Later it streams > > > in the count and calls init_tree_ssa again, this time with correct > > > count, which is rounded up to 50 and allocated again leaking the first > > > array. > > > > > > This patch fixes it by adding extra argument to init_tree_ssa specifing > > > whether ssa names should be initialized. There are few places where we > > > initialize ssa and only lto streaming is special, so i think extra arg > > > works well here. > > > > > > I am not quite sure about the 50MB default. I think it was made before > > > ggc_free was implemented (so resizing vectors was expensive) and when we > > > had only one function in SSA form at a time. Most of functions in C++ > > > will probably need fewer than 50 SSA names until inlining. > > > > > > This saves 21MB of WPA linking libxul. > > > > Why do we input SSA names at WPA? > > To ICF compare function bodies. It tends to load tons of small bodies. > We also may load body when merging BB profile etc. > > > > > Bootstrapping/regtesting x86_64-linux, OK if it suceeds? > > > > I think calling init_ssanames again is bogus. Please simply > > do that in input_ssa_names after we know how many we need > > adding a number of SSA names to reserve argument to init_tree_ssa > > and passing that down instead. > > I tried that originally but it hit ICE in init_ssa_operands. Seems that > reloacating them is last change I need though. Does this look better? Patch below looks completely unrelated but is OK as well if it passes testing ;) Richard. > * tree-cfg.c (remove_bb): Release SSA defs. > * tree-ssa.c (verify_flow_sensitive_alias_info): Do not walk dead nodes. > * tree-tailcall.c (eliminate_tail_call): Release SSA name. > Index: tree-cfg.c > =================================================================== > RCS file: /cvs/gcc/gcc/gcc/tree-cfg.c,v > retrieving revision 2.47 > diff -c -3 -p -r2.47 tree-cfg.c > *** tree-cfg.c 6 Sep 2004 10:07:56 -0000 2.47 > --- tree-cfg.c 7 Sep 2004 13:26:46 -0000 > *************** remove_bb (basic_block bb) > *** 1818,1823 **** > --- 1818,1824 ---- > for (i = bsi_start (bb); !bsi_end_p (i); bsi_remove (&i)) > { > tree stmt = bsi_stmt (i); > + release_defs (stmt); > > set_bb_for_stmt (stmt, NULL); > > Index: tree-tailcall.c > =================================================================== > RCS file: /cvs/gcc/gcc/gcc/tree-tailcall.c,v > retrieving revision 2.23 > diff -c -3 -p -r2.23 tree-tailcall.c > *** tree-tailcall.c 6 Sep 2004 10:08:13 -0000 2.23 > --- tree-tailcall.c 7 Sep 2004 13:26:47 -0000 > *************** eliminate_tail_call (struct tailcall *t) > *** 681,692 **** > bsi_next (&bsi); > while (!bsi_end_p (bsi)) > { > /* Do not remove the return statement, so that redirect_edge_and_branch > sees how the block ends. */ > ! if (TREE_CODE (bsi_stmt (bsi)) == RETURN_EXPR) > break; > > bsi_remove (&bsi); > } > > /* Replace the call by a jump to the start of function. */ > --- 681,694 ---- > bsi_next (&bsi); > while (!bsi_end_p (bsi)) > { > + tree t = bsi_stmt (bsi); > /* Do not remove the return statement, so that redirect_edge_and_branch > sees how the block ends. */ > ! if (TREE_CODE (t) == RETURN_EXPR) > break; > > bsi_remove (&bsi); > + release_defs (t); > } > > /* Replace the call by a jump to the start of function. */ > *************** eliminate_tail_call (struct tailcall *t) > *** 772,777 **** > --- 774,780 ---- > } > > bsi_remove (&t->call_bsi); > + release_defs (call); > } > > /* Optimizes the tailcall described by T. If OPT_TAILCALLS is true, also > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imend ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do not leak memory when streaming in ssa names 2020-11-23 12:49 ` Richard Biener @ 2020-11-23 12:56 ` Jan Hubicka 2020-11-23 13:18 ` Jan Hubicka 2020-11-23 13:18 ` Richard Biener 0 siblings, 2 replies; 7+ messages in thread From: Jan Hubicka @ 2020-11-23 12:56 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > > > > Hi, > > > > tree-streamer-in currently calls init_tree_ssa that calls init_ssanames > > > > that allocates space in ssa_names array for 50 names. Later it streams > > > > in the count and calls init_tree_ssa again, this time with correct > > > > count, which is rounded up to 50 and allocated again leaking the first > > > > array. > > > > > > > > This patch fixes it by adding extra argument to init_tree_ssa specifing > > > > whether ssa names should be initialized. There are few places where we > > > > initialize ssa and only lto streaming is special, so i think extra arg > > > > works well here. > > > > > > > > I am not quite sure about the 50MB default. I think it was made before > > > > ggc_free was implemented (so resizing vectors was expensive) and when we > > > > had only one function in SSA form at a time. Most of functions in C++ > > > > will probably need fewer than 50 SSA names until inlining. > > > > > > > > This saves 21MB of WPA linking libxul. > > > > > > Why do we input SSA names at WPA? > > > > To ICF compare function bodies. It tends to load tons of small bodies. > > We also may load body when merging BB profile etc. > > > > > > > Bootstrapping/regtesting x86_64-linux, OK if it suceeds? > > > > > > I think calling init_ssanames again is bogus. Please simply > > > do that in input_ssa_names after we know how many we need > > > adding a number of SSA names to reserve argument to init_tree_ssa > > > and passing that down instead. > > > > I tried that originally but it hit ICE in init_ssa_operands. Seems that > > reloacating them is last change I need though. Does this look better? > > Patch below looks completely unrelated but is OK as well if it passes > testing ;) Glad to hear since it is in mainline for a while :) Here is correct pathc. Sorry. * lto-streamer-in.c (input_cfg): Do not init ssa operands. (input_function): Do not init tree_ssa and set in_ssa_p. (input_ssa_names): Do it here. * tree-ssa.c (init_tree_ssa): Add additional SIZE parameter, default to 0 * tree-ssanames.c (init_ssanames): Do not round size up to 50, allocate precisely. diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c index 64037f74ad3..a20d64b0089 100644 --- a/gcc/lto-streamer-in.c +++ b/gcc/lto-streamer-in.c @@ -1017,7 +1017,6 @@ input_cfg (class lto_input_block *ib, class data_in *data_in, int index; init_empty_tree_cfg_for_function (fn); - init_ssa_operands (fn); profile_status_for_fn (fn) = streamer_read_enum (ib, profile_status_d, PROFILE_LAST); @@ -1144,7 +1143,9 @@ input_ssa_names (class lto_input_block *ib, class data_in *data_in, unsigned int i, size; size = streamer_read_uhwi (ib); - init_ssanames (fn, size); + init_tree_ssa (fn, size); + cfun->gimple_df->in_ssa_p = true; + init_ssa_operands (fn); i = streamer_read_uhwi (ib); while (i) @@ -1384,9 +1385,6 @@ input_function (tree fn_decl, class data_in *data_in, push_struct_function (fn_decl); fn = DECL_STRUCT_FUNCTION (fn_decl); - init_tree_ssa (fn); - /* We input IL in SSA form. */ - cfun->gimple_df->in_ssa_p = true; gimple_register_cfg_hooks (); diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c index b44361f8244..24a5154620d 100644 --- a/gcc/tree-ssa.c +++ b/gcc/tree-ssa.c @@ -1218,15 +1218,16 @@ err: # pragma GCC diagnostic pop #endif -/* Initialize global DFA and SSA structures. */ +/* Initialize global DFA and SSA structures. + If SIZE is non-zero allocated ssa names array of a given size. */ void -init_tree_ssa (struct function *fn) +init_tree_ssa (struct function *fn, int size) { fn->gimple_df = ggc_cleared_alloc<gimple_df> (); fn->gimple_df->default_defs = hash_table<ssa_name_hasher>::create_ggc (20); pt_solution_reset (&fn->gimple_df->escaped); - init_ssanames (fn, 0); + init_ssanames (fn, size); } /* Deallocate memory associated with SSA data structures for FNDECL. */ diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c index 6ac97fe39c4..ec4681f85cb 100644 --- a/gcc/tree-ssanames.c +++ b/gcc/tree-ssanames.c @@ -77,10 +77,10 @@ unsigned int ssa_name_nodes_created; void init_ssanames (struct function *fn, int size) { - if (size < 50) - size = 50; - - vec_alloc (SSANAMES (fn), size); + if (!size) + vec_alloc (SSANAMES (fn), 50); + else + vec_safe_reserve (SSANAMES (fn), size, true); /* Version 0 is special, so reserve the first slot in the table. Though currently unused, we may use version 0 in alias analysis as part of ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do not leak memory when streaming in ssa names 2020-11-23 12:56 ` Jan Hubicka @ 2020-11-23 13:18 ` Jan Hubicka 2020-11-23 13:18 ` Richard Biener 1 sibling, 0 replies; 7+ messages in thread From: Jan Hubicka @ 2020-11-23 13:18 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches > > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > > > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > > > > > > Hi, > > > > > tree-streamer-in currently calls init_tree_ssa that calls init_ssanames > > > > > that allocates space in ssa_names array for 50 names. Later it streams > > > > > in the count and calls init_tree_ssa again, this time with correct > > > > > count, which is rounded up to 50 and allocated again leaking the first > > > > > array. > > > > > > > > > > This patch fixes it by adding extra argument to init_tree_ssa specifing > > > > > whether ssa names should be initialized. There are few places where we > > > > > initialize ssa and only lto streaming is special, so i think extra arg > > > > > works well here. > > > > > > > > > > I am not quite sure about the 50MB default. I think it was made before > > > > > ggc_free was implemented (so resizing vectors was expensive) and when we > > > > > had only one function in SSA form at a time. Most of functions in C++ > > > > > will probably need fewer than 50 SSA names until inlining. > > > > > > > > > > This saves 21MB of WPA linking libxul. I actually was overly pesimistic here. It changes: GGC memory Leak Garbage Freed Overhead Times -------------------------------------------------------------------------------------------------------------------------------------------- Total 2116M:100.0% 779M:100.0% 3744M:100.0% 312M:100.0% 49M -------------------------------------------------------------------------------------------------------------------------------------------- to -------------------------------------------------------------------------------------------------------------------------------------------- GGC memory Leak Garbage Freed Overhead Times -------------------------------------------------------------------------------------------------------------------------------------------- Total 2137M:100.0% 916M:100.0% 3813M:100.0% 390M:100.0% 49M -------------------------------------------------------------------------------------------------------------------------------------------- So in addition to 21MB of leak memory (i.e. function bodies still needed) also 137MB of garbage (i.e. functions eliminated by ICF or dead code elimination) Honza ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do not leak memory when streaming in ssa names 2020-11-23 12:56 ` Jan Hubicka 2020-11-23 13:18 ` Jan Hubicka @ 2020-11-23 13:18 ` Richard Biener 1 sibling, 0 replies; 7+ messages in thread From: Richard Biener @ 2020-11-23 13:18 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On Mon, 23 Nov 2020, Jan Hubicka wrote: > > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > > > On Mon, 23 Nov 2020, Jan Hubicka wrote: > > > > > > > > > Hi, > > > > > tree-streamer-in currently calls init_tree_ssa that calls init_ssanames > > > > > that allocates space in ssa_names array for 50 names. Later it streams > > > > > in the count and calls init_tree_ssa again, this time with correct > > > > > count, which is rounded up to 50 and allocated again leaking the first > > > > > array. > > > > > > > > > > This patch fixes it by adding extra argument to init_tree_ssa specifing > > > > > whether ssa names should be initialized. There are few places where we > > > > > initialize ssa and only lto streaming is special, so i think extra arg > > > > > works well here. > > > > > > > > > > I am not quite sure about the 50MB default. I think it was made before > > > > > ggc_free was implemented (so resizing vectors was expensive) and when we > > > > > had only one function in SSA form at a time. Most of functions in C++ > > > > > will probably need fewer than 50 SSA names until inlining. > > > > > > > > > > This saves 21MB of WPA linking libxul. > > > > > > > > Why do we input SSA names at WPA? > > > > > > To ICF compare function bodies. It tends to load tons of small bodies. > > > We also may load body when merging BB profile etc. > > > > > > > > > Bootstrapping/regtesting x86_64-linux, OK if it suceeds? > > > > > > > > I think calling init_ssanames again is bogus. Please simply > > > > do that in input_ssa_names after we know how many we need > > > > adding a number of SSA names to reserve argument to init_tree_ssa > > > > and passing that down instead. > > > > > > I tried that originally but it hit ICE in init_ssa_operands. Seems that > > > reloacating them is last change I need though. Does this look better? > > > > Patch below looks completely unrelated but is OK as well if it passes > > testing ;) > > Glad to hear since it is in mainline for a while :) > Here is correct pathc. Sorry. LGTM. Thanks, Richard. > * lto-streamer-in.c (input_cfg): Do not init ssa operands. > (input_function): Do not init tree_ssa and set in_ssa_p. > (input_ssa_names): Do it here. > * tree-ssa.c (init_tree_ssa): Add additional SIZE parameter, default > to 0 > * tree-ssanames.c (init_ssanames): Do not round size up to 50, allocate > precisely. > diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c > index 64037f74ad3..a20d64b0089 100644 > --- a/gcc/lto-streamer-in.c > +++ b/gcc/lto-streamer-in.c > @@ -1017,7 +1017,6 @@ input_cfg (class lto_input_block *ib, class data_in *data_in, > int index; > > init_empty_tree_cfg_for_function (fn); > - init_ssa_operands (fn); > > profile_status_for_fn (fn) = streamer_read_enum (ib, profile_status_d, > PROFILE_LAST); > @@ -1144,7 +1143,9 @@ input_ssa_names (class lto_input_block *ib, class data_in *data_in, > unsigned int i, size; > > size = streamer_read_uhwi (ib); > - init_ssanames (fn, size); > + init_tree_ssa (fn, size); > + cfun->gimple_df->in_ssa_p = true; > + init_ssa_operands (fn); > > i = streamer_read_uhwi (ib); > while (i) > @@ -1384,9 +1385,6 @@ input_function (tree fn_decl, class data_in *data_in, > > push_struct_function (fn_decl); > fn = DECL_STRUCT_FUNCTION (fn_decl); > - init_tree_ssa (fn); > - /* We input IL in SSA form. */ > - cfun->gimple_df->in_ssa_p = true; > > gimple_register_cfg_hooks (); > > diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c > index b44361f8244..24a5154620d 100644 > --- a/gcc/tree-ssa.c > +++ b/gcc/tree-ssa.c > @@ -1218,15 +1218,16 @@ err: > # pragma GCC diagnostic pop > #endif > > -/* Initialize global DFA and SSA structures. */ > +/* Initialize global DFA and SSA structures. > + If SIZE is non-zero allocated ssa names array of a given size. */ > > void > -init_tree_ssa (struct function *fn) > +init_tree_ssa (struct function *fn, int size) > { > fn->gimple_df = ggc_cleared_alloc<gimple_df> (); > fn->gimple_df->default_defs = hash_table<ssa_name_hasher>::create_ggc (20); > pt_solution_reset (&fn->gimple_df->escaped); > - init_ssanames (fn, 0); > + init_ssanames (fn, size); > } > > /* Deallocate memory associated with SSA data structures for FNDECL. */ > diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c > index 6ac97fe39c4..ec4681f85cb 100644 > --- a/gcc/tree-ssanames.c > +++ b/gcc/tree-ssanames.c > @@ -77,10 +77,10 @@ unsigned int ssa_name_nodes_created; > void > init_ssanames (struct function *fn, int size) > { > - if (size < 50) > - size = 50; > - > - vec_alloc (SSANAMES (fn), size); > + if (!size) > + vec_alloc (SSANAMES (fn), 50); > + else > + vec_safe_reserve (SSANAMES (fn), size, true); > > /* Version 0 is special, so reserve the first slot in the table. Though > currently unused, we may use version 0 in alias analysis as part of > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imend ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-23 13:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-23 12:18 Do not leak memory when streaming in ssa names Jan Hubicka 2020-11-23 12:27 ` Richard Biener 2020-11-23 12:43 ` Jan Hubicka 2020-11-23 12:49 ` Richard Biener 2020-11-23 12:56 ` Jan Hubicka 2020-11-23 13:18 ` Jan Hubicka 2020-11-23 13:18 ` 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).