Re: [PATCH 1/3] Refactor copying decl section names
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
Re: [PATCH 1/3] Refactor copying decl section names
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. -- Alan Modra Australia Development Lab, IBM
Re: [PATCH 1/3] Refactor copying decl section names
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
Re: [PATCH 1/3] Refactor copying decl section names
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. -- Alan Modra Australia Development Lab, IBM
Re: [PATCH 1/3] Refactor copying decl section names
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
Re: [PATCH 1/3] Refactor copying decl section names
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 > > * 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
Re: [PATCH 1/3] Refactor copying decl section names
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 > > * 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 >
[PATCH 1/3] Refactor copying decl section names
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
* 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.
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 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.
