| Message ID | 0383ee3e-ce9e-5268-db13-bc2deae23f83@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Come up with constructors of symtab_node, cgraph_node and varpool_node. | expand |
On Thu, Dec 5, 2019 at 1:50 PM Martin Liška <mliska@suse.cz> wrote: > > Hi. > > As mentioned in the PR, there are classes in cgraph.h that are > not PODs and are initialized with ggc_alloc_cleared. So that I'm suggesting > to use proper constructors. I added ggc_new function that can be used > at different locations as well. Isn't there ggc_alloc <T> for this? Also ggc_alloc_no_dtor<T> in case you want to handle finalization yourself. > I'm attaching optimized dump file with how ctor expansion looks like. > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > gcc/ChangeLog: > > 2019-12-05 Martin Liska <mliska@suse.cz> > > PR ipa/92737 > * cgraph.c (symbol_table_test::symbol_table_test): > Use new ggc_new. > * cgraph.h (symtab_node::symtab_node): New constructor. > (cgraph_node::cgraph_node): Likewise. > (varpool_node::varpool_node): Likewise. > (symbol_table::allocate_cgraph_symbol): Use newly > created constructor. > * cgraphunit.c (symtab_terminator): Likewise. > * ggc.h (ggc_new): New. > * toplev.c (general_init): Use new ggc_new. > * varpool.c (varpool_node::create_empty): Use newly > created constructor. > > gcc/c-family/ChangeLog: > > 2019-12-05 Martin Liska <mliska@suse.cz> > > PR ipa/92737 > * c-opts.c (c_common_init_options): Use new ggc_new. > --- > gcc/c-family/c-opts.c | 4 +--- > gcc/cgraph.c | 2 +- > gcc/cgraph.h | 50 ++++++++++++++++++++++++++++++++++++------- > gcc/cgraphunit.c | 2 +- > gcc/ggc.h | 9 ++++++++ > gcc/toplev.c | 2 +- > gcc/varpool.c | 5 ++--- > 7 files changed, 57 insertions(+), 17 deletions(-) > >
> Hi. > > As mentioned in the PR, there are classes in cgraph.h that are > not PODs and are initialized with ggc_alloc_cleared. So that I'm suggesting > to use proper constructors. I added ggc_new function that can be used > at different locations as well. > > I'm attaching optimized dump file with how ctor expansion looks like. > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? Thanks for working on this! I filled to PR to not forget to do this (since it can trigger wrong code if ggc allocation gets inlined) > + /* Default constructor. */ > + symtab_node (symtab_type t) > + : type (t), resolution (LDPR_UNKNOWN), definition (0), alias (0), > + transparent_alias (0), weakref (0), cpp_implicit_alias (0), symver (0), > + analyzed (0), writeonly (0), refuse_visibility_changes (0), > + externally_visible (0), no_reorder (0), force_output (0), > + forced_by_abi (0), unique_name (0), implicit_section (0), > + body_removed (0), used_from_other_partition (0), in_other_partition (0), > + address_taken (0), in_init_priority_hash (0), need_lto_streaming (0), > + offloadable (0), ifunc_resolver (0), order (0), decl (NULL_TREE), I would use (false) for definition...order since these are flags. I think there is no need to initialize decl, next, previous since all allocations immediately put things into the symbol table and these items are always initialized explicitly. > + /* Default constructor. */ > + cgraph_node (int uid) > + : symtab_node (SYMTAB_FUNCTION), callees (NULL), callers (NULL), > + indirect_calls (NULL), origin (NULL), nested (NULL), next_nested (NULL), > + next_sibling_clone (NULL), prev_sibling_clone (NULL), clones (NULL), > + clone_of (NULL), call_site_hash (NULL), former_clone_of (NULL), > + simdclone (NULL), simd_clones (NULL), ipa_transforms_to_apply (), > + inlined_to (NULL), rtl (NULL), clone (), thunk (), count (), Will ipa_transformas_to_appl end up being NULL? count is initialized to profile_count::uninitialized in cgraph_node::created that also can eb moved here. > + count_materialization_scale (0), profile_id (0), unit_id (0), > + tp_first_run (0), used_as_abstract_origin (0), lowered (0), process (0), > + frequency (), only_called_at_startup (0), only_called_at_exit (0), > + tm_clone (0), dispatcher_function (0), calls_comdat_local (0), > + icf_merged (0), nonfreeing_fn (0), merged_comdat (0), > + merged_extern_inline (0), parallelized_function (0), split_part (0), > + indirect_call_target (0), local (0), versionable (0), > + can_change_signature (0), redefined_extern_inline (0), > + tm_may_enter_irr (0), ipcp_clone (0), m_uid (uid), m_summary_id (-1) Again flags used_as_abstract_oriign...ipcp_clone are better as (false). OK with these changes. Honza
On 12/5/19 1:59 PM, Richard Biener wrote: > Isn't there ggc_alloc <T> for this? Also ggc_alloc_no_dtor<T> in case you > want to handle finalization yourself. No, if I see correctly it only calls Dtor: template<typename T> inline T * ggc_alloc (ALONE_CXX_MEM_STAT_INFO) { if (need_finalization_p<T> ()) return static_cast<T *> (ggc_internal_alloc (sizeof (T), finalize<T>, 0, 1 PASS_MEM_STAT)); else return static_cast<T *> (ggc_internal_alloc (sizeof (T), NULL, 0, 1 PASS_MEM_STAT)); } ... /* Allocate a chunk of memory of SIZE bytes. Its contents are undefined. */ void * ggc_internal_alloc (size_t size, void (*f)(void *), size_t s, size_t n MEM_STAT_DECL) { ... The function is generic, it does not know about type T. That's why it does not (and can't call) ctor. Martin
On December 5, 2019 2:03:58 PM GMT+01:00, "Martin Liška" <mliska@suse.cz> wrote: >On 12/5/19 1:59 PM, Richard Biener wrote: >> Isn't there ggc_alloc <T> for this? Also ggc_alloc_no_dtor<T> in >case you >> want to handle finalization yourself. > >No, if I see correctly it only calls Dtor: But its odd to handle finalization but not construction here and not finalization in your new wrapper. That doesn't look like a good API. I see Honza approved the change but maybe you can fix the actual issue without this new API for the moment. Richard. >template<typename T> >inline T * >ggc_alloc (ALONE_CXX_MEM_STAT_INFO) >{ > if (need_finalization_p<T> ()) >return static_cast<T *> (ggc_internal_alloc (sizeof (T), finalize<T>, >0, 1 > PASS_MEM_STAT)); > else > return static_cast<T *> (ggc_internal_alloc (sizeof (T), NULL, 0, 1 > PASS_MEM_STAT)); >} > >... > >/* Allocate a chunk of memory of SIZE bytes. Its contents are >undefined. */ > >void * >ggc_internal_alloc (size_t size, void (*f)(void *), size_t s, size_t n > MEM_STAT_DECL) >{ >... > >The function is generic, it does not know about type T. That's why it >does not (and can't call) ctor. > >Martin
> On December 5, 2019 2:03:58 PM GMT+01:00, "Martin Liška" <mliska@suse.cz> wrote: > >On 12/5/19 1:59 PM, Richard Biener wrote: > >> Isn't there ggc_alloc <T> for this? Also ggc_alloc_no_dtor<T> in > >case you > >> want to handle finalization yourself. > > > >No, if I see correctly it only calls Dtor: > > But its odd to handle finalization but not construction here and not finalization in your new wrapper. > > That doesn't look like a good API. I see Honza approved the change but maybe you can fix the actual issue without this new API for the moment. Sorry, I was too fast here - I meant to comment on the ggc_new as well. Indeed current scheme seems bit confusing to me, too. Honza
On 12/5/19 2:53 PM, Richard Biener wrote: > On December 5, 2019 2:03:58 PM GMT+01:00, "Martin Liška" <mliska@suse.cz> wrote: >> On 12/5/19 1:59 PM, Richard Biener wrote: >>> Isn't there ggc_alloc <T> for this? Also ggc_alloc_no_dtor<T> in >> case you >>> want to handle finalization yourself. >> >> No, if I see correctly it only calls Dtor: > > But its odd to handle finalization but not construction here and not finalization in your new wrapper. No, ggc_new calls ggc_alloc that will eventually call destructor for types with non-trivial destructors. Alternative solution would be to no come up with ggc_alloc and call default ctor in ggc_alloc. But that's too restrictive as all types must have default constructor. That's not desirable to me. Martin > > That doesn't look like a good API. I see Honza approved the change but maybe you can fix the actual issue without this new API for the moment. > > Richard. > >> template<typename T> >> inline T * >> ggc_alloc (ALONE_CXX_MEM_STAT_INFO) >> { >> if (need_finalization_p<T> ()) >> return static_cast<T *> (ggc_internal_alloc (sizeof (T), finalize<T>, >> 0, 1 >> PASS_MEM_STAT)); >> else >> return static_cast<T *> (ggc_internal_alloc (sizeof (T), NULL, 0, 1 >> PASS_MEM_STAT)); >> } >> >> ... >> >> /* Allocate a chunk of memory of SIZE bytes. Its contents are >> undefined. */ >> >> void * >> ggc_internal_alloc (size_t size, void (*f)(void *), size_t s, size_t n >> MEM_STAT_DECL) >> { >> ... >> >> The function is generic, it does not know about type T. That's why it >> does not (and can't call) ctor. >> >> Martin >
>>>>> "Martin" == Martin Liška <mliska@suse.cz> writes:
Martin> + /* Default constructor. */
Martin> + symtab_node (symtab_type t)
FWIW, in gdb, we normally make single-argument constructors "explicit".
This helps avoid surprises with implicit conversions.
Tom
On December 5, 2019 3:09:40 PM GMT+01:00, "Martin Liška" <mliska@suse.cz> wrote: >On 12/5/19 2:53 PM, Richard Biener wrote: >> On December 5, 2019 2:03:58 PM GMT+01:00, "Martin Liška" ><mliska@suse.cz> wrote: >>> On 12/5/19 1:59 PM, Richard Biener wrote: >>>> Isn't there ggc_alloc <T> for this? Also ggc_alloc_no_dtor<T> in >>> case you >>>> want to handle finalization yourself. >>> >>> No, if I see correctly it only calls Dtor: >> >> But its odd to handle finalization but not construction here and not >finalization in your new wrapper. > >No, ggc_new calls ggc_alloc that will eventually call destructor for >types >with non-trivial destructors. > >Alternative solution would be to no come up with ggc_alloc and call >default >ctor in ggc_alloc. But that's too restrictive as all types must have >default >constructor. That's not desirable to me. Isn't there std::default_constructible? Also after your patch it's far from obvious which api to use. Richard. >Martin > >> >> That doesn't look like a good API. I see Honza approved the change >but maybe you can fix the actual issue without this new API for the >moment. >> >> Richard. >> >>> template<typename T> >>> inline T * >>> ggc_alloc (ALONE_CXX_MEM_STAT_INFO) >>> { >>> if (need_finalization_p<T> ()) >>> return static_cast<T *> (ggc_internal_alloc (sizeof (T), >finalize<T>, >>> 0, 1 >>> PASS_MEM_STAT)); >>> else >>> return static_cast<T *> (ggc_internal_alloc (sizeof (T), NULL, >0, 1 >>> PASS_MEM_STAT)); >>> } >>> >>> ... >>> >>> /* Allocate a chunk of memory of SIZE bytes. Its contents are >>> undefined. */ >>> >>> void * >>> ggc_internal_alloc (size_t size, void (*f)(void *), size_t s, size_t >n >>> MEM_STAT_DECL) >>> { >>> ... >>> >>> The function is generic, it does not know about type T. That's why >it >>> does not (and can't call) ctor. >>> >>> Martin >>
On December 5, 2019 3:35:17 PM GMT+01:00, Tom Tromey <tom@tromey.com> wrote: >>>>>> "Martin" == Martin Liška <mliska@suse.cz> writes: > >Martin> + /* Default constructor. */ >Martin> + symtab_node (symtab_type t) > >FWIW, in gdb, we normally make single-argument constructors "explicit". >This helps avoid surprises with implicit conversions. Indeed - please adjust that as well. Richard >Tom
On 12/5/19 2:03 PM, Jan Hubicka wrote: >> Hi. >> >> As mentioned in the PR, there are classes in cgraph.h that are >> not PODs and are initialized with ggc_alloc_cleared. So that I'm suggesting >> to use proper constructors. I added ggc_new function that can be used >> at different locations as well. >> >> I'm attaching optimized dump file with how ctor expansion looks like. >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? > Thanks for working on this! I filled to PR to not forget to do this > (since it can trigger wrong code if ggc allocation gets inlined) >> + /* Default constructor. */ >> + symtab_node (symtab_type t) >> + : type (t), resolution (LDPR_UNKNOWN), definition (0), alias (0), >> + transparent_alias (0), weakref (0), cpp_implicit_alias (0), symver (0), >> + analyzed (0), writeonly (0), refuse_visibility_changes (0), >> + externally_visible (0), no_reorder (0), force_output (0), >> + forced_by_abi (0), unique_name (0), implicit_section (0), >> + body_removed (0), used_from_other_partition (0), in_other_partition (0), >> + address_taken (0), in_init_priority_hash (0), need_lto_streaming (0), >> + offloadable (0), ifunc_resolver (0), order (0), decl (NULL_TREE), > I would use (false) for definition...order since these are flags. > I think there is no need to initialize decl, next, previous since all > allocations immediately put things into the symbol table and these items > are always initialized explicitly. >> + /* Default constructor. */ >> + cgraph_node (int uid) >> + : symtab_node (SYMTAB_FUNCTION), callees (NULL), callers (NULL), >> + indirect_calls (NULL), origin (NULL), nested (NULL), next_nested (NULL), >> + next_sibling_clone (NULL), prev_sibling_clone (NULL), clones (NULL), >> + clone_of (NULL), call_site_hash (NULL), former_clone_of (NULL), >> + simdclone (NULL), simd_clones (NULL), ipa_transforms_to_apply (), >> + inlined_to (NULL), rtl (NULL), clone (), thunk (), count (), > > Will ipa_transformas_to_appl end up being NULL? > count is initialized to profile_count::uninitialized in > cgraph_node::created that also can eb moved here. > >> + count_materialization_scale (0), profile_id (0), unit_id (0), >> + tp_first_run (0), used_as_abstract_origin (0), lowered (0), process (0), >> + frequency (), only_called_at_startup (0), only_called_at_exit (0), >> + tm_clone (0), dispatcher_function (0), calls_comdat_local (0), >> + icf_merged (0), nonfreeing_fn (0), merged_comdat (0), >> + merged_extern_inline (0), parallelized_function (0), split_part (0), >> + indirect_call_target (0), local (0), versionable (0), >> + can_change_signature (0), redefined_extern_inline (0), >> + tm_may_enter_irr (0), ipcp_clone (0), m_uid (uid), m_summary_id (-1) > Again flags used_as_abstract_oriign...ipcp_clone are better as (false). > > OK with these changes. > Honza > Hello. I included all the Honza's notes and made some additional clean up. What remains to be resolved is the ggc_new function. I'm testing this updated patch. Martin
On 12/5/19 4:12 PM, Richard Biener wrote: > Isn't there std::default_constructible? Also after your patch it's far from obvious which api to use. Will it work with: struct Foo { Foo(int) {} }; ... if (std::default_constructible<T> ()) ptr = new ptr T (); ? Wouldn't we end up with a compilation error that T() does not exist? Well, I don't insist on the ggc_new function. I can easily leave it. Martin > > Richard.
On 12/5/19 4:17 PM, Martin Liška wrote: > On 12/5/19 4:12 PM, Richard Biener wrote: >> Isn't there std::default_constructible? Also after your patch it's far from obvious which api to use. > > Will it work with: > > struct Foo > { > Foo(int) {} > }; > > > ... > if (std::default_constructible<T> ()) > ptr = new ptr T (); > > ? > Wouldn't we end up with a compilation error that T() does not exist? > > Well, I don't insist on the ggc_new function. I can easily leave it. > > Martin > >> >> Richard. > Anyway there's updated patch that I'm testing. Martin
On 12/5/19 8:13 AM, Martin Liška wrote: > On 12/5/19 2:03 PM, Jan Hubicka wrote: >>> Hi. >>> >>> As mentioned in the PR, there are classes in cgraph.h that are >>> not PODs and are initialized with ggc_alloc_cleared. So that I'm >>> suggesting >>> to use proper constructors. I added ggc_new function that can be used >>> at different locations as well. >>> >>> I'm attaching optimized dump file with how ctor expansion looks like. >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>> >>> Ready to be installed? index 9c086fedaef..b7dea696782 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -109,6 +109,23 @@ struct GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"), public: friend class symbol_table; + /* Default constructor. */ + symtab_node (symtab_type t) Since it takes an argument the above is not the default ctor. It can be made one by providing a default argument for t, such as NULL_TREE if that's valid. I don't know the details of how these things are defined now (POD vs non-POD) or how they're used (where POD is expected) but having been bitten a bunch of times recently by various GCC container templates making assumptions about their elements being PODs, it seems worth pointing it out here. If without this change symtab_node is a POD, adding one could cause problems. Same for the other structs. + : type (t), resolution (LDPR_UNKNOWN), definition (false), alias (false), + transparent_alias (false), weakref (false), cpp_implicit_alias (false), + symver (false), analyzed (false), writeonly (false), + refuse_visibility_changes (false), externally_visible (false), + no_reorder (false), force_output (false), forced_by_abi (false), + unique_name (false), implicit_section (false), body_removed (false), + used_from_other_partition (false), in_other_partition (false), + address_taken (false), in_init_priority_hash (false), + need_lto_streaming (false), offloadable (false), ifunc_resolver (false), + order (false), next_sharing_asm_name (NULL), + previous_sharing_asm_name (NULL), same_comdat_group (NULL), ref_list (), + alias_target (NULL), lto_file_data (NULL), aux (NULL), + x_comdat_group (NULL_TREE), x_section (NULL) + {} This is a matter of personal preference but for whatever it's worth, I like using default zero initialization for zero- initialized members, e.g., definition (), alias (), ... alias_target (), ... Besides being less verbose it has the advantage that changing the type of the member doesn't need to require changing its initializer. (An argument for the more verbose form might be that it makes the initial value immediately clear.) Martin
Hi, On Thu, 5 Dec 2019, Richard Biener wrote: > On December 5, 2019 3:35:17 PM GMT+01:00, Tom Tromey <tom@tromey.com> wrote: > >>>>>> "Martin" == Martin Liška <mliska@suse.cz> writes: > > > >Martin> + /* Default constructor. */ > >Martin> + symtab_node (symtab_type t) > > > >FWIW, in gdb, we normally make single-argument constructors "explicit". > >This helps avoid surprises with implicit conversions. > > Indeed - please adjust that as well. Explicit ctors are a c++11+ feature. Ciao, Michael.
On December 5, 2019 5:31:59 PM GMT+01:00, Michael Matz <matz@suse.de> wrote: >Hi, > >On Thu, 5 Dec 2019, Richard Biener wrote: > >> On December 5, 2019 3:35:17 PM GMT+01:00, Tom Tromey <tom@tromey.com> >wrote: >> >>>>>> "Martin" == Martin Liška <mliska@suse.cz> writes: >> > >> >Martin> + /* Default constructor. */ >> >Martin> + symtab_node (symtab_type t) >> > >> >FWIW, in gdb, we normally make single-argument constructors >"explicit". >> >This helps avoid surprises with implicit conversions. >> >> Indeed - please adjust that as well. > >Explicit ctors are a c++11+ feature. Surely not. Richard. > >Ciao, >Michael.
Hi, On Thu, 5 Dec 2019, Richard Biener wrote: > >> Indeed - please adjust that as well. > > > >Explicit ctors are a c++11+ feature. > > Surely not. Whoops, I was conflating ctors and conversion functions, the latter can be explicit only in c++11+. Ciao, Michael.
On 5 December 2019 16:24:53 CET, "Martin Liška" <mliska@suse.cz> wrote:
-/* Allocate new callgraph node. */
-
-inline cgraph_node *
-symbol_table::allocate_cgraph_symbol (void)
-{
- cgraph_node *node;
-
- node = ggc_cleared_alloc<cgraph_node> ();
- node->type = SYMTAB_FUNCTION;
- node->m_summary_id = -1;
- node->m_uid = cgraph_max_uid++;
- return node;
-}
Just because I don't see it in the patch, how is cgraph_max_uid++ maintained after that patch?
thanks,
On 12/7/19 12:49 AM, Bernhard Reutner-Fischer wrote: > On 5 December 2019 16:24:53 CET, "Martin Liška" <mliska@suse.cz> wrote: > > -/* Allocate new callgraph node. */ > - > -inline cgraph_node * > -symbol_table::allocate_cgraph_symbol (void) > -{ > - cgraph_node *node; > - > - node = ggc_cleared_alloc<cgraph_node> (); > - node->type = SYMTAB_FUNCTION; > - node->m_summary_id = -1; > - node->m_uid = cgraph_max_uid++; > - return node; > -} > > Just because I don't see it in the patch, how is cgraph_max_uid++ maintained after that patch? It's moved to function symbol_table::create_empty (void) where we call: return new (ggc_alloc<cgraph_node> ()) cgraph_node (cgraph_max_uid++); Thanks, Martin > > thanks, >
On 12/5/19 5:25 PM, Martin Sebor wrote: > On 12/5/19 8:13 AM, Martin Liška wrote: >> On 12/5/19 2:03 PM, Jan Hubicka wrote: >>>> Hi. >>>> >>>> As mentioned in the PR, there are classes in cgraph.h that are >>>> not PODs and are initialized with ggc_alloc_cleared. So that I'm suggesting >>>> to use proper constructors. I added ggc_new function that can be used >>>> at different locations as well. >>>> >>>> I'm attaching optimized dump file with how ctor expansion looks like. >>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>>> >>>> Ready to be installed? > > index 9c086fedaef..b7dea696782 100644 > --- a/gcc/cgraph.h > +++ b/gcc/cgraph.h > @@ -109,6 +109,23 @@ struct GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"), > public: > friend class symbol_table; Hello. > > + /* Default constructor. */ > + symtab_node (symtab_type t) I've adjusted this in the patch. > > Since it takes an argument the above is not the default ctor. > It can be made one by providing a default argument for t, such > as NULL_TREE if that's valid. > > I don't know the details of how these things are defined now > (POD vs non-POD) or how they're used (where POD is expected) > but having been bitten a bunch of times recently by various > GCC container templates making assumptions about their > elements being PODs, it seems worth pointing it out here. > If without this change symtab_node is a POD, adding one > could cause problems. Same for the other structs. > > + : type (t), resolution (LDPR_UNKNOWN), definition (false), alias (false), > + transparent_alias (false), weakref (false), cpp_implicit_alias (false), > + symver (false), analyzed (false), writeonly (false), > + refuse_visibility_changes (false), externally_visible (false), > + no_reorder (false), force_output (false), forced_by_abi (false), > + unique_name (false), implicit_section (false), body_removed (false), > + used_from_other_partition (false), in_other_partition (false), > + address_taken (false), in_init_priority_hash (false), > + need_lto_streaming (false), offloadable (false), ifunc_resolver (false), > + order (false), next_sharing_asm_name (NULL), > + previous_sharing_asm_name (NULL), same_comdat_group (NULL), ref_list (), > + alias_target (NULL), lto_file_data (NULL), aux (NULL), > + x_comdat_group (NULL_TREE), x_section (NULL) > + {} > > This is a matter of personal preference but for whatever it's > worth, I like using default zero initialization for zero- > initialized members, e.g., > > definition (), alias (), ... alias_target (), ... > > Besides being less verbose it has the advantage that changing > the type of the member doesn't need to require changing its > initializer. (An argument for the more verbose form might > be that it makes the initial value immediately clear.) Well, I do prefer the more explicit form where we will use 'false' as default value for various flag member variables. I'm going to install the following patch. Martin > > Martin
diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c index c913291c07c..a8d9ddd17dc 100644 --- a/gcc/c-family/c-opts.c +++ b/gcc/c-family/c-opts.c @@ -222,9 +222,7 @@ c_common_init_options (unsigned int decoded_options_count, unsigned int i; struct cpp_callbacks *cb; - g_string_concat_db - = new (ggc_alloc <string_concat_db> ()) string_concat_db (); - + g_string_concat_db = ggc_new<string_concat_db> (); parse_in = cpp_create_reader (c_dialect_cxx () ? CLK_GNUCXX: CLK_GNUC89, ident_hash, line_table); cb = cpp_get_callbacks (parse_in); diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 7288440708e..0c83215c596 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -3750,7 +3750,7 @@ symbol_table_test::symbol_table_test () { gcc_assert (saved_symtab == NULL); saved_symtab = symtab; - symtab = new (ggc_alloc <symbol_table> ()) symbol_table (); + symtab = ggc_new<symbol_table> (); } /* Destructor. Restore the old value of symtab. */ diff --git a/gcc/cgraph.h b/gcc/cgraph.h index 9c086fedaef..700c333128c 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -109,6 +109,22 @@ struct GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"), public: friend class symbol_table; + /* Default constructor. */ + symtab_node (symtab_type t) + : type (t), resolution (LDPR_UNKNOWN), definition (0), alias (0), + transparent_alias (0), weakref (0), cpp_implicit_alias (0), symver (0), + analyzed (0), writeonly (0), refuse_visibility_changes (0), + externally_visible (0), no_reorder (0), force_output (0), + forced_by_abi (0), unique_name (0), implicit_section (0), + body_removed (0), used_from_other_partition (0), in_other_partition (0), + address_taken (0), in_init_priority_hash (0), need_lto_streaming (0), + offloadable (0), ifunc_resolver (0), order (0), decl (NULL_TREE), + next (NULL), previous (NULL), next_sharing_asm_name (NULL), + previous_sharing_asm_name (NULL), same_comdat_group (NULL), ref_list (), + alias_target (NULL), lto_file_data (NULL), aux (NULL), + x_comdat_group (NULL_TREE), x_section (NULL) + {} + /* Return name. */ const char *name () const; @@ -901,6 +917,25 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node { friend class symbol_table; + /* Default constructor. */ + cgraph_node (int uid) + : symtab_node (SYMTAB_FUNCTION), callees (NULL), callers (NULL), + indirect_calls (NULL), origin (NULL), nested (NULL), next_nested (NULL), + next_sibling_clone (NULL), prev_sibling_clone (NULL), clones (NULL), + clone_of (NULL), call_site_hash (NULL), former_clone_of (NULL), + simdclone (NULL), simd_clones (NULL), ipa_transforms_to_apply (), + inlined_to (NULL), rtl (NULL), clone (), thunk (), count (), + count_materialization_scale (0), profile_id (0), unit_id (0), + tp_first_run (0), used_as_abstract_origin (0), lowered (0), process (0), + frequency (), only_called_at_startup (0), only_called_at_exit (0), + tm_clone (0), dispatcher_function (0), calls_comdat_local (0), + icf_merged (0), nonfreeing_fn (0), merged_comdat (0), + merged_extern_inline (0), parallelized_function (0), split_part (0), + indirect_call_target (0), local (0), versionable (0), + can_change_signature (0), redefined_extern_inline (0), + tm_may_enter_irr (0), ipcp_clone (0), m_uid (uid), m_summary_id (-1) + {} + /* Remove the node from cgraph and all inline clones inlined into it. Skip however removal of FORBIDDEN_NODE and return true if it needs to be removed. This allows to call the function from outer loop walking clone @@ -1877,6 +1912,12 @@ private: struct GTY((tag ("SYMTAB_VARIABLE"))) varpool_node : public symtab_node { + /* Default constructor. */ + varpool_node () + : symtab_node (SYMTAB_VARIABLE), output (0), dynamically_initialized (0), + tls_model (TLS_MODEL_NONE), used_by_single_function (0) + {} + /* Dump given varpool node to F. */ void dump (FILE *f); @@ -2721,16 +2762,9 @@ symbol_table::release_symbol (cgraph_node *node) inline cgraph_node * symbol_table::allocate_cgraph_symbol (void) { - cgraph_node *node; - - node = ggc_cleared_alloc<cgraph_node> (); - node->type = SYMTAB_FUNCTION; - node->m_summary_id = -1; - node->m_uid = cgraph_max_uid++; - return node; + return new (ggc_alloc<cgraph_node> ()) cgraph_node (cgraph_max_uid++); } - /* Return first static symbol with definition. */ inline symtab_node * symbol_table::first_symbol (void) diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c index 1b3d2812152..b80071cad99 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -274,7 +274,7 @@ symtab_node::needed_p (void) /* Head and terminator of the queue of nodes to be processed while building callgraph. */ -static symtab_node symtab_terminator; +static symtab_node symtab_terminator (SYMTAB_SYMBOL); static symtab_node *queued_nodes = &symtab_terminator; /* Add NODE to queue starting at QUEUED_NODES. diff --git a/gcc/ggc.h b/gcc/ggc.h index 6c64caaafb2..11022a36324 100644 --- a/gcc/ggc.h +++ b/gcc/ggc.h @@ -185,6 +185,15 @@ ggc_alloc (ALONE_CXX_MEM_STAT_INFO) PASS_MEM_STAT)); } +/* Allocate GGC memory of type T and call default constructor. */ + +template<typename T> +inline T * +ggc_new (ALONE_CXX_MEM_STAT_INFO) +{ + return new (ggc_alloc<T> ()) T (); +} + /* GGC allocation function that does not call finalizer for type that have need_finalization_p equal to true. User is responsible for calling of the destructor. */ diff --git a/gcc/toplev.c b/gcc/toplev.c index 059046f40f3..26af9b81fb8 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1243,7 +1243,7 @@ general_init (const char *argv0, bool init_signals) /* Create the passes. */ g->set_passes (new gcc::pass_manager (g)); - symtab = new (ggc_alloc <symbol_table> ()) symbol_table (); + symtab = ggc_new<symbol_table> (); statistics_early_init (); debuginfo_early_init (); diff --git a/gcc/varpool.c b/gcc/varpool.c index 1a30ae49d54..f313acc51f2 100644 --- a/gcc/varpool.c +++ b/gcc/varpool.c @@ -133,9 +133,8 @@ symbol_table::call_varpool_insertion_hooks (varpool_node *node) varpool_node * varpool_node::create_empty (void) -{ - varpool_node *node = ggc_cleared_alloc<varpool_node> (); - node->type = SYMTAB_VARIABLE; +{ + varpool_node *node = ggc_new<varpool_node> (); return node; }
Hi. As mentioned in the PR, there are classes in cgraph.h that are not PODs and are initialized with ggc_alloc_cleared. So that I'm suggesting to use proper constructors. I added ggc_new function that can be used at different locations as well. I'm attaching optimized dump file with how ctor expansion looks like. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/ChangeLog: 2019-12-05 Martin Liska <mliska@suse.cz> PR ipa/92737 * cgraph.c (symbol_table_test::symbol_table_test): Use new ggc_new. * cgraph.h (symtab_node::symtab_node): New constructor. (cgraph_node::cgraph_node): Likewise. (varpool_node::varpool_node): Likewise. (symbol_table::allocate_cgraph_symbol): Use newly created constructor. * cgraphunit.c (symtab_terminator): Likewise. * ggc.h (ggc_new): New. * toplev.c (general_init): Use new ggc_new. * varpool.c (varpool_node::create_empty): Use newly created constructor. gcc/c-family/ChangeLog: 2019-12-05 Martin Liska <mliska@suse.cz> PR ipa/92737 * c-opts.c (c_common_init_options): Use new ggc_new. --- gcc/c-family/c-opts.c | 4 +--- gcc/cgraph.c | 2 +- gcc/cgraph.h | 50 ++++++++++++++++++++++++++++++++++++------- gcc/cgraphunit.c | 2 +- gcc/ggc.h | 9 ++++++++ gcc/toplev.c | 2 +- gcc/varpool.c | 5 ++--- 7 files changed, 57 insertions(+), 17 deletions(-) varpool_node::create_empty () { struct varpool_node * _4; unsigned long _5; unsigned long _8; unsigned long _9; unsigned char _64; unsigned char _65; unsigned char _66; <bb 2> [local count: 1073741824]: _4 = ggc_internal_alloc (128, 0B, 0, 1); _9 = MEM[(struct symtab_node *)_4]; _8 = _9 & 18446743523953737728; _5 = _8 | 2; MEM[(struct symtab_node *)_4] = _5; MEM[(struct symtab_node *)_4].order = 0; MEM[(struct symtab_node *)_4].decl = 0B; MEM[(struct symtab_node *)_4].next = 0B; MEM[(struct symtab_node *)_4].previous = 0B; MEM[(struct symtab_node *)_4].next_sharing_asm_name = 0B; MEM[(struct symtab_node *)_4].previous_sharing_asm_name = 0B; MEM[(struct symtab_node *)_4].same_comdat_group = 0B; MEM[(struct symtab_node *)_4].ref_list.references = 0B; MEM[(struct symtab_node *)_4].ref_list.referring.m_vec = 0B; MEM[(struct symtab_node *)_4].alias_target = 0B; MEM[(struct symtab_node *)_4].lto_file_data = 0B; MEM[(struct symtab_node *)_4].aux = 0B; MEM[(struct symtab_node *)_4].x_comdat_group = 0B; MEM[(struct symtab_node *)_4].x_section = 0B; _64 = MEM[(struct varpool_node *)_4 + 120B]; _65 = _64 & 192; _66 = _65 | 0; MEM[(struct varpool_node *)_4 + 120B] = _66; return _4; } cgraph_node::create (union tree_node * decl) { ... _55 = ggc_internal_alloc (360, 0B, 0, 1); _56 = &_55->D.104276; _71 = MEM[(struct symtab_node *)_55]; _70 = _71 & 18446743523953737728; _168 = _70 | 1; MEM[(struct symtab_node *)_55] = _168; MEM[(struct symtab_node *)_55].order = 0; MEM[(struct symtab_node *)_55].decl = 0B; MEM[(struct symtab_node *)_55].next = 0B; MEM[(struct symtab_node *)_55].previous = 0B; MEM[(struct symtab_node *)_55].next_sharing_asm_name = 0B; MEM[(struct symtab_node *)_55].previous_sharing_asm_name = 0B; MEM[(struct symtab_node *)_55].same_comdat_group = 0B; MEM[(struct symtab_node *)_55].ref_list.references = 0B; MEM[(struct symtab_node *)_55].ref_list.referring.m_vec = 0B; MEM[(struct symtab_node *)_55].alias_target = 0B; MEM[(struct symtab_node *)_55].lto_file_data = 0B; MEM[(struct symtab_node *)_55].aux = 0B; MEM[(struct symtab_node *)_55].x_comdat_group = 0B; MEM[(struct symtab_node *)_55].x_section = 0B; *_55.callees = 0B; *_55.callers = 0B; *_55.indirect_calls = 0B; *_55.origin = 0B; *_55.nested = 0B; *_55.next_nested = 0B; *_55.next_sibling_clone = 0B; *_55.prev_sibling_clone = 0B; *_55.clones = 0B; *_55.clone_of = 0B; *_55.call_site_hash = 0B; *_55.former_clone_of = 0B; *_55.simdclone = 0B; *_55.simd_clones = 0B; *_55.ipa_transforms_to_apply.m_vec = 0B; *_55.inlined_to = 0B; *_55.rtl = 0B; *_55.clone.tree_map = 0B; *_55.clone.param_adjustments = 0B; *_55.clone.performed_splits = 0B; *_55.thunk = {}; MEM[(struct profile_count *)_55 + 320B] = 0; *_55.m_uid = _53; *_55.m_summary_id = -1; MEM[(void *)_55 + 328B] = 10000; MEM[(int *)_55 + 336B] = 0; _162 = MEM[(struct cgraph_node *)_55 + 344B]; _37 = _162 & 4286578688; _65 = _37 | 16; MEM[(struct cgraph_node *)_55 + 344B] = _65; _29 = symtab.55_1->cgraph_count; _25 = _29 + 1; symtab.55_1->cgraph_count = _25; ...