| Message ID | CAC-ggsEvufEh9-cm2FtB+OFa3gwK1uuH1-A9d_=aJtJrpZPjqw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] Refactor copying decl section names | expand |
On Tue, 2019-11-12 at 22:28 -0800, Strager Neds wrote: > Sometimes, we need to copy a section name from one decl or symtab node > to another. Currently, this is done by getting the source's section > name and setting the destination's section name. For example: > > set_decl_section_name (dest, DECL_SECTION_NAME (source)); > dest->set_section (source->get_section ()); > > This code could be more efficient. Section names are stored in an > interning hash table, but the current interfaces of > set_decl_section_name and symtab_node::set_section force unnecessary > indirections (to get the section name) and hashing (to find the section > name in the hash table). > > Overload set_decl_section_name and symtab_node::set_section to accept an > existing symtab_node to copy the section name from: > > set_decl_section_name (dest, source); > dest->set_section (*source); > > For now, implement these new functions as a simple wrapper around the > existing functions. In the future, these functions can be implemented > using just a pointer copy and an increment (for the reference count). > > This patch should not change behavior. > > Testing: Bootstrap on x86_64-linux-gnu with --disable-multilib > --enable-checking=release --enable-languages=c,c++. Observe no change in > test results. > > 2019-11-12 Matthew Glazar <strager.nds@gmail.com> > > * gcc/cgraph.h (symtab_node::get_section): Constify. > (symtab_node::set_section): Declare new overload. > * gcc/symtab.c (symtab_node::set_section): Define new overload. > (symtab_node::copy_visibility_from): Use new overload of > symtab_node::set_section. > (symtab_node::resolve_alias): Same. > * gcc/tree.h (set_decl_section_name): Declare new overload. > * gcc/tree.c (set_decl_section_name): Define new overload. > * gcc/c/c-decl.c (merge_decls): Use new overload of > set_decl_section_name. > * gcc/cp/decl.c (duplicate_decls): Same. > * gcc/cp/method.c (use_thunk): Same. > * gcc/cp/optimize.c (maybe_clone_body): Same. > * gcc/d/decl.cc (finish_thunk): Same. > * gcc/tree-emutls.c (get_emutls_init_templ_addr): Same. > * gcc/cgraphclones.c (cgraph_node::create_virtual_clone): Use new > overload of symtab_node::set_section. > (cgraph_node::create_version_clone_with_body): Same. > * gcc/trans-mem.c (ipa_tm_create_version): Same. [ ... ] I know these were sent when we were still in stage1, but they fell through the cracks. I'm deferring the kit until gcc-11 stage1. jeff >
On 11/12/19 11:28 PM, Strager Neds wrote: > Sometimes, we need to copy a section name from one decl or symtab node > to another. Currently, this is done by getting the source's section > name and setting the destination's section name. For example: > > set_decl_section_name (dest, DECL_SECTION_NAME (source)); > dest->set_section (source->get_section ()); > > This code could be more efficient. Section names are stored in an > interning hash table, but the current interfaces of > set_decl_section_name and symtab_node::set_section force unnecessary > indirections (to get the section name) and hashing (to find the section > name in the hash table). > > Overload set_decl_section_name and symtab_node::set_section to accept an > existing symtab_node to copy the section name from: > > set_decl_section_name (dest, source); > dest->set_section (*source); > > For now, implement these new functions as a simple wrapper around the > existing functions. In the future, these functions can be implemented > using just a pointer copy and an increment (for the reference count). > > This patch should not change behavior. > > Testing: Bootstrap on x86_64-linux-gnu with --disable-multilib > --enable-checking=release --enable-languages=c,c++. Observe no change in > test results. > > 2019-11-12 Matthew Glazar <strager.nds@gmail.com> > > * gcc/cgraph.h (symtab_node::get_section): Constify. > (symtab_node::set_section): Declare new overload. > * gcc/symtab.c (symtab_node::set_section): Define new overload. > (symtab_node::copy_visibility_from): Use new overload of > symtab_node::set_section. > (symtab_node::resolve_alias): Same. > * gcc/tree.h (set_decl_section_name): Declare new overload. > * gcc/tree.c (set_decl_section_name): Define new overload. > * gcc/c/c-decl.c (merge_decls): Use new overload of > set_decl_section_name. > * gcc/cp/decl.c (duplicate_decls): Same. > * gcc/cp/method.c (use_thunk): Same. > * gcc/cp/optimize.c (maybe_clone_body): Same. > * gcc/d/decl.cc (finish_thunk): Same. > * gcc/tree-emutls.c (get_emutls_init_templ_addr): Same. > * gcc/cgraphclones.c (cgraph_node::create_virtual_clone): Use new > overload of symtab_node::set_section. > (cgraph_node::create_version_clone_with_body): Same. > * gcc/trans-mem.c (ipa_tm_create_version): Same. So only a year after the original post.... Presumably given that you're just adding an overload, there is no need or intent to change all the callers to set_decl_section_name. Right? I scanned the various remaining calls, some just use a string, or build up a string based on a decl or something else. I did find one other call that should be converted in cp/coroutines.cc which almost certainly went onto the trunk after your patch was submitted. I've adjusted that one. Also note, your mailer seemed to mangle whitespace in your message. It makes it slightly harder to deal with, so if you plan on making regular contributions, we should probably work to get that fixed. If the refactoring + subsequent bugfixes are all you're planning to do, then I'll fix the whitespace issues manually. Given the age of the patch, the need to manually apply bits (due to whitespace issues) and the desire to fix coroutines.cc, I'm giving it a fresh test spin. Jeff
On 11/12/19 11:28 PM, Strager Neds wrote: > * gcc/cgraph.h (symtab_node::get_section): Constify. > (symtab_node::set_section): Declare new overload. > * gcc/symtab.c (symtab_node::set_section): Define new overload. > (symtab_node::copy_visibility_from): Use new overload of > symtab_node::set_section. > (symtab_node::resolve_alias): Same. > * gcc/tree.h (set_decl_section_name): Declare new overload. > * gcc/tree.c (set_decl_section_name): Define new overload. > * gcc/c/c-decl.c (merge_decls): Use new overload of > set_decl_section_name. > * gcc/cp/decl.c (duplicate_decls): Same. > * gcc/cp/method.c (use_thunk): Same. > * gcc/cp/optimize.c (maybe_clone_body): Same. > * gcc/d/decl.cc (finish_thunk): Same. > * gcc/tree-emutls.c (get_emutls_init_templ_addr): Same. > * gcc/cgraphclones.c (cgraph_node::create_virtual_clone): Use new > overload of symtab_node::set_section. > (cgraph_node::create_version_clone_with_body): Same. > * gcc/trans-mem.c (ipa_tm_create_version): Same. I adjusted the ChangeLog, added an entry for the coroutines.cc addition I made and pushed this to the trunk. Thanks for your patience. Jeff
On Tue, Nov 10, 2020 at 11:45:37AM -0700, Jeff Law wrote: > > On 11/12/19 11:28 PM, Strager Neds wrote: > > * gcc/cgraph.h (symtab_node::get_section): Constify. > > (symtab_node::set_section): Declare new overload. > > * gcc/symtab.c (symtab_node::set_section): Define new overload. > > (symtab_node::copy_visibility_from): Use new overload of > > symtab_node::set_section. > > (symtab_node::resolve_alias): Same. > > * gcc/tree.h (set_decl_section_name): Declare new overload. > > * gcc/tree.c (set_decl_section_name): Define new overload. > > * gcc/c/c-decl.c (merge_decls): Use new overload of > > set_decl_section_name. > > * gcc/cp/decl.c (duplicate_decls): Same. > > * gcc/cp/method.c (use_thunk): Same. > > * gcc/cp/optimize.c (maybe_clone_body): Same. > > * gcc/d/decl.cc (finish_thunk): Same. > > * gcc/tree-emutls.c (get_emutls_init_templ_addr): Same. > > * gcc/cgraphclones.c (cgraph_node::create_virtual_clone): Use new > > overload of symtab_node::set_section. > > (cgraph_node::create_version_clone_with_body): Same. > > * gcc/trans-mem.c (ipa_tm_create_version): Same. > > I adjusted the ChangeLog, added an entry for the coroutines.cc addition > I made and pushed this to the trunk. /home/alan/src/gcc/gcc/go/go-gcc.cc:2759:44: error: call of overloaded ‘set_decl_section_name(tree_node*&, std::nullptr_t)’ is ambiguous set_decl_section_name (var_decl, NULL); ^ In file included from /home/alan/src/gcc/gcc/go/go-gcc.cc:27:0: /home/alan/src/gcc/gcc/tree.h:4283:13: note: candidate: void set_decl_section_name(tree, const char*) extern void set_decl_section_name (tree, const char *); ^~~~~~~~~~~~~~~~~~~~~ /home/alan/src/gcc/gcc/tree.h:4284:13: note: candidate: void set_decl_section_name(tree, const_tree) extern void set_decl_section_name (tree, const_tree); ^~~~~~~~~~~~~~~~~~~~~ I'm using the obvious to me "(const char *) NULL" in the call to fix this, but you might like a different style C++ fix instead.
On 11/10/20 8:57 PM, Alan Modra wrote: > On Tue, Nov 10, 2020 at 11:45:37AM -0700, Jeff Law wrote: >> On 11/12/19 11:28 PM, Strager Neds wrote: >>> * gcc/cgraph.h (symtab_node::get_section): Constify. >>> (symtab_node::set_section): Declare new overload. >>> * gcc/symtab.c (symtab_node::set_section): Define new overload. >>> (symtab_node::copy_visibility_from): Use new overload of >>> symtab_node::set_section. >>> (symtab_node::resolve_alias): Same. >>> * gcc/tree.h (set_decl_section_name): Declare new overload. >>> * gcc/tree.c (set_decl_section_name): Define new overload. >>> * gcc/c/c-decl.c (merge_decls): Use new overload of >>> set_decl_section_name. >>> * gcc/cp/decl.c (duplicate_decls): Same. >>> * gcc/cp/method.c (use_thunk): Same. >>> * gcc/cp/optimize.c (maybe_clone_body): Same. >>> * gcc/d/decl.cc (finish_thunk): Same. >>> * gcc/tree-emutls.c (get_emutls_init_templ_addr): Same. >>> * gcc/cgraphclones.c (cgraph_node::create_virtual_clone): Use new >>> overload of symtab_node::set_section. >>> (cgraph_node::create_version_clone_with_body): Same. >>> * gcc/trans-mem.c (ipa_tm_create_version): Same. >> I adjusted the ChangeLog, added an entry for the coroutines.cc addition >> I made and pushed this to the trunk. > /home/alan/src/gcc/gcc/go/go-gcc.cc:2759:44: error: call of overloaded ‘set_decl_section_name(tree_node*&, std::nullptr_t)’ is ambiguous > set_decl_section_name (var_decl, NULL); > ^ > In file included from /home/alan/src/gcc/gcc/go/go-gcc.cc:27:0: > /home/alan/src/gcc/gcc/tree.h:4283:13: note: candidate: void set_decl_section_name(tree, const char*) > extern void set_decl_section_name (tree, const char *); > ^~~~~~~~~~~~~~~~~~~~~ > /home/alan/src/gcc/gcc/tree.h:4284:13: note: candidate: void set_decl_section_name(tree, const_tree) > extern void set_decl_section_name (tree, const_tree); > ^~~~~~~~~~~~~~~~~~~~~ > > I'm using the obvious to me "(const char *) NULL" in the call to fix > this, but you might like a different style C++ fix instead. Ugh. I don't typically build go. Sorry about that. Funny thing is I saw the NULL when I was looking for other instances of set_decl_section_name that should have been converted. But the fact that it would trigger an ambiguous overload didn't cross my mind. I think the const char * is fine. That should force resolution to the same routine we were using earlier. Do you want to commit that fix or shall I? jeff
On Tue, Nov 10, 2020 at 09:19:37PM -0700, Jeff Law wrote: > I think the const char * is fine. That should force resolution to the > same routine we were using earlier. Do you want to commit that fix or > shall I? Commited 693a79a355e1.
On 11/10/20 10:11 PM, Alan Modra wrote: > On Tue, Nov 10, 2020 at 09:19:37PM -0700, Jeff Law wrote: >> I think the const char * is fine. That should force resolution to the >> same routine we were using earlier. Do you want to commit that fix or >> shall I? > Commited 693a79a355e1. THanks. I ended up not coming back to the computer last night, so it's definitely good you took it :-) jeff
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 2841b4f5a77..366fbf2a28a 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -2845,7 +2845,7 @@ merge_decls (tree newdecl, tree olddecl, tree newtype, tree oldtype) || TREE_PUBLIC (olddecl) || TREE_STATIC (olddecl)) && DECL_SECTION_NAME (newdecl) != NULL) - set_decl_section_name (olddecl, DECL_SECTION_NAME (newdecl)); + set_decl_section_name (olddecl, newdecl); /* This isn't quite correct for something like int __thread x attribute ((tls_model ("local-exec"))); diff --git a/gcc/cgraph.h b/gcc/cgraph.h index 0abde3d8f91..3b07258b31d 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -246,7 +246,7 @@ public: } /* Return section as string. */ - const char * get_section () + const char * get_section () const { if (!x_section) return NULL; @@ -305,6 +305,9 @@ public: /* Set section for symbol and its aliases. */ void set_section (const char *section); + /* Like set_section, but copying the section name from another node. */ + void set_section (const symtab_node &other); + /* Set section, do not recurse into aliases. When one wants to change section of symbol and its aliases, use set_section. */ diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c index 41a600e64a5..0b1c93534f2 100644 --- a/gcc/cgraphclones.c +++ b/gcc/cgraphclones.c @@ -572,7 +572,7 @@ cgraph_node::create_virtual_clone (vec<cgraph_edge *> redirect_callers, set_new_clone_decl_and_node_flags (new_node); new_node->clone.tree_map = tree_map; if (!implicit_section) - new_node->set_section (get_section ()); + new_node->set_section (*this); /* Clones of global symbols or symbols with unique names are unique. */ if ((TREE_PUBLIC (old_decl) @@ -996,7 +996,7 @@ cgraph_node::create_version_clone_with_body new_version_node->local = 1; new_version_node->lowered = true; if (!implicit_section) - new_version_node->set_section (get_section ()); + new_version_node->set_section (*this); /* Clones of global symbols or symbols with unique names are unique. */ if ((TREE_PUBLIC (old_decl) && !DECL_EXTERNAL (old_decl) diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 5c5a85e3221..ed4034f8e9d 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -2830,7 +2830,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend) done later in decl_attributes since we are called before attributes are assigned. */ if (DECL_SECTION_NAME (newdecl) != NULL) - set_decl_section_name (olddecl, DECL_SECTION_NAME (newdecl)); + set_decl_section_name (olddecl, newdecl); if (DECL_ONE_ONLY (newdecl)) { diff --git a/gcc/cp/method.c b/gcc/cp/method.c index 47441c10c52..d111792af5b 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -351,7 +351,7 @@ use_thunk (tree thunk_fndecl, bool emit_p) resolve_unique_section (thunk_fndecl, 0, flag_function_sections); /* Output the thunk into the same section as function. */ - set_decl_section_name (thunk_fndecl, DECL_SECTION_NAME (fn)); + set_decl_section_name (thunk_fndecl, fn); symtab_node::get (thunk_fndecl)->implicit_section = symtab_node::get (fn)->implicit_section; } diff --git a/gcc/cp/optimize.c b/gcc/cp/optimize.c index 994ee9148c0..707ccdafcdb 100644 --- a/gcc/cp/optimize.c +++ b/gcc/cp/optimize.c @@ -518,7 +518,7 @@ maybe_clone_body (tree fn) DECL_DLLIMPORT_P (clone) = DECL_DLLIMPORT_P (fn); DECL_ATTRIBUTES (clone) = copy_list (DECL_ATTRIBUTES (fn)); DECL_DISREGARD_INLINE_LIMITS (clone) = DECL_DISREGARD_INLINE_LIMITS (fn); - set_decl_section_name (clone, DECL_SECTION_NAME (fn)); + set_decl_section_name (clone, fn); /* Adjust the parameter names and locations. */ parm = DECL_ARGUMENTS (fn); diff --git a/gcc/d/decl.cc b/gcc/d/decl.cc index bcce245e59c..d6c934b1163 100644 --- a/gcc/d/decl.cc +++ b/gcc/d/decl.cc @@ -1677,7 +1677,7 @@ finish_thunk (tree thunk, tree function) resolve_unique_section (thunk, 0, flag_function_sections); /* Output the thunk into the same section as function. */ - set_decl_section_name (thunk, DECL_SECTION_NAME (fn)); + set_decl_section_name (thunk, fn); symtab_node::get (thunk)->implicit_section = symtab_node::get (fn)->implicit_section; } diff --git a/gcc/symtab.c b/gcc/symtab.c index 3e634e22c86..84d17c36189 100644 --- a/gcc/symtab.c +++ b/gcc/symtab.c @@ -1418,8 +1418,7 @@ symtab_node::copy_visibility_from (symtab_node *n) DECL_DLLIMPORT_P (decl) = DECL_DLLIMPORT_P (n->decl); resolution = n->resolution; set_comdat_group (n->get_comdat_group ()); - call_for_symbol_and_aliases (symtab_node::set_section, - const_cast<char *>(n->get_section ()), true); + set_section (*n); externally_visible = n->externally_visible; if (!DECL_RTL_SET_P (decl)) return; @@ -1605,6 +1604,14 @@ symtab_node::set_section (const char *section) (symtab_node::set_section, const_cast<char *>(section), true); } +void +symtab_node::set_section (const symtab_node &other) +{ + const char *section = other.get_section (); + call_for_symbol_and_aliases + (symtab_node::set_section, const_cast<char *>(section), true); +} + /* Return the initialization priority. */ priority_type @@ -1748,8 +1755,7 @@ symtab_node::resolve_alias (symtab_node *target, bool transparent) { error ("section of alias %q+D must match section of its target", decl); } - call_for_symbol_and_aliases (symtab_node::set_section, - const_cast<char *>(target->get_section ()), true); + set_section (*target); if (target->implicit_section) call_for_symbol_and_aliases (set_implicit_section, NULL, true); diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c index 2e775286540..a2023d60ea0 100644 --- a/gcc/trans-mem.c +++ b/gcc/trans-mem.c @@ -4994,7 +4994,7 @@ ipa_tm_create_version (struct cgraph_node *old_node) new_node->lowered = true; new_node->tm_clone = 1; if (!old_node->implicit_section) - new_node->set_section (old_node->get_section ()); + new_node->set_section (*old_node); get_cg_data (&old_node, true)->clone = new_node; if (old_node->get_availability () >= AVAIL_INTERPOSABLE) diff --git a/gcc/tree-emutls.c b/gcc/tree-emutls.c index 6fc8370f42c..b7b0a57bde4 100644 --- a/gcc/tree-emutls.c +++ b/gcc/tree-emutls.c @@ -259,7 +259,7 @@ get_emutls_init_templ_addr (tree decl) if (targetm.emutls.tmpl_section) set_decl_section_name (to, targetm.emutls.tmpl_section); else - set_decl_section_name (to, DECL_SECTION_NAME (decl)); + set_decl_section_name (to, decl); /* Create varpool node for the new variable and finalize it if it is not external one. */ diff --git a/gcc/tree.c b/gcc/tree.c index d2c9fe35995..0c5b88302b7 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -774,6 +774,33 @@ set_decl_section_name (tree node, const char *value) snode->set_section (value); } +/* Set section name of NODE to match the section name of OTHER. + + set_decl_section_name (decl, other) is equivalent to + set_decl_section_name (decl, DECL_SECTION_NAME (other)), but possibly more + efficient. */ +void +set_decl_section_name (tree decl, const_tree other) +{ + struct symtab_node *other_node = symtab_node::get (other); + if (other_node) + { + struct symtab_node *decl_node; + if (VAR_P (decl)) + decl_node = varpool_node::get_create (decl); + else + decl_node = cgraph_node::get_create (decl); + decl_node->set_section (*other_node); + } + else + { + struct symtab_node *decl_node = symtab_node::get (decl); + if (!decl_node) + return; + decl_node->set_section (NULL); + } +} + /* Return TLS model of a variable NODE. */ enum tls_model decl_tls_model (const_tree node) diff --git a/gcc/tree.h b/gcc/tree.h index a7d39c3a74d..91e519cdb83 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -4241,6 +4241,7 @@ extern tree decl_comdat_group (const_tree); extern tree decl_comdat_group_id (const_tree); extern const char *decl_section_name (const_tree); extern void set_decl_section_name (tree, const char *); +extern void set_decl_section_name (tree, const_tree); extern enum tls_model decl_tls_model (const_tree); extern void set_decl_tls_model (tree, enum tls_model);
Sometimes, we need to copy a section name from one decl or symtab node to another. Currently, this is done by getting the source's section name and setting the destination's section name. For example: set_decl_section_name (dest, DECL_SECTION_NAME (source)); dest->set_section (source->get_section ()); This code could be more efficient. Section names are stored in an interning hash table, but the current interfaces of set_decl_section_name and symtab_node::set_section force unnecessary indirections (to get the section name) and hashing (to find the section name in the hash table). Overload set_decl_section_name and symtab_node::set_section to accept an existing symtab_node to copy the section name from: set_decl_section_name (dest, source); dest->set_section (*source); For now, implement these new functions as a simple wrapper around the existing functions. In the future, these functions can be implemented using just a pointer copy and an increment (for the reference count). This patch should not change behavior. Testing: Bootstrap on x86_64-linux-gnu with --disable-multilib --enable-checking=release --enable-languages=c,c++. Observe no change in test results. 2019-11-12 Matthew Glazar <strager.nds@gmail.com> * gcc/cgraph.h (symtab_node::get_section): Constify. (symtab_node::set_section): Declare new overload. * gcc/symtab.c (symtab_node::set_section): Define new overload. (symtab_node::copy_visibility_from): Use new overload of symtab_node::set_section. (symtab_node::resolve_alias): Same. * gcc/tree.h (set_decl_section_name): Declare new overload. * gcc/tree.c (set_decl_section_name): Define new overload. * gcc/c/c-decl.c (merge_decls): Use new overload of set_decl_section_name. * gcc/cp/decl.c (duplicate_decls): Same. * gcc/cp/method.c (use_thunk): Same. * gcc/cp/optimize.c (maybe_clone_body): Same. * gcc/d/decl.cc (finish_thunk): Same. * gcc/tree-emutls.c (get_emutls_init_templ_addr): Same. * gcc/cgraphclones.c (cgraph_node::create_virtual_clone): Use new overload of symtab_node::set_section. (cgraph_node::create_version_clone_with_body): Same. * gcc/trans-mem.c (ipa_tm_create_version): Same.