[PATCH 1/3] Refactor copying decl section names

[PATCH 1/3] Refactor copying decl section names

[Strager Neds]

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.


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);

[PATCH 2/3] Refactor section name ref counting

[Strager Neds]

symtab_node::set_section_for_node manages the reference count of
section_hash_entry objects. I plan to add another function which needs
to manage the reference count of these objects. To avoid duplicating
code, factor the existing logic into reusable functions.

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/symtab.c (symtab_node::set_section_for_node): Extract reference
counting logic into ...
(retain_section_hash_entry): ... here (new function) and ...
(release_section_hash_entry): ... here (new function).


diff --git a/gcc/symtab.c b/gcc/symtab.c
index 84d17c36189..a2aa519e760 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -368,6 +368,30 @@ section_name_hasher::equal (section_hash_entry
*n1, const char *name)
   return n1->name == name || !strcmp (n1->name, name);
 }

+static section_hash_entry *
+retain_section_hash_entry (section_hash_entry *entry)
+{
+  entry->ref_count++;
+  return entry;
+}
+
+static void
+release_section_hash_entry (section_hash_entry *entry)
+{
+  if (entry)
+    {
+      entry->ref_count--;
+      if (!entry->ref_count)
+    {
+      hashval_t hash = htab_hash_string (entry->name);
+      section_hash_entry **slot =
symtab->section_hash->find_slot_with_hash (entry->name,
+                                hash, INSERT);
+      ggc_free (entry);
+      symtab->section_hash->clear_slot (slot);
+    }
+    }
+}
+
 /* Add node into symbol table.  This function is not used directly, but via
    cgraph/varpool node creation routines.  */

@@ -1543,46 +1567,33 @@ void
 symtab_node::set_section_for_node (const char *section)
 {
   const char *current = get_section ();
-  section_hash_entry **slot;

   if (current == section
       || (current && section
       && !strcmp (current, section)))
     return;

-  if (current)
-    {
-      x_section->ref_count--;
-      if (!x_section->ref_count)
-    {
-      hashval_t hash = htab_hash_string (x_section->name);
-      slot = symtab->section_hash->find_slot_with_hash (x_section->name,
-                                hash, INSERT);
-      ggc_free (x_section);
-      symtab->section_hash->clear_slot (slot);
-    }
-      x_section = NULL;
-    }
+  release_section_hash_entry (x_section);
   if (!section)
     {
+      x_section = NULL;
       implicit_section = false;
       return;
     }
   if (!symtab->section_hash)
     symtab->section_hash = hash_table<section_name_hasher>::create_ggc (10);
-  slot = symtab->section_hash->find_slot_with_hash (section,
-                            htab_hash_string (section),
-                            INSERT);
+  section_hash_entry **slot = symtab->section_hash->find_slot_with_hash
+    (section, htab_hash_string (section), INSERT);
   if (*slot)
-    x_section = (section_hash_entry *)*slot;
+    x_section = retain_section_hash_entry (*slot);
   else
     {
       int len = strlen (section);
       *slot = x_section = ggc_cleared_alloc<section_hash_entry> ();
+      x_section->ref_count = 1;
       x_section->name = ggc_vec_alloc<char> (len + 1);
       memcpy (x_section->name, section, len + 1);
     }
-  x_section->ref_count++;
 }

 /* Worker for set_section.  */

[PATCH 3/3] Improve efficiency of copying section from another tree

[Strager Neds]

Several parts of GCC need to copy a section name from one tree (or
symtab_node) to another. Currently, this is implemented naively:

1. Query the source's section name
2. Hash the section name string
3. Find the section_hash_entry in the symbol table
4. Increment the section_hash_entry's reference count
5. Assign the destination's section to the section_hash_entry

Since we have the source's section_hash_entry, we can copy the section
name from one symtab_node to another efficiently with the following
algorithm:

1. Query the source's section_hash_entry
2. Increment the section_hash_entry's reference count
3. Assign the destination's section to the section_hash_entry

Implement this algorithm in the overload of symtab_node::set_section
which takes an existing symtab_node.

I did not measure the performance impact of this patch. In
particular, I do not know if this patch actually improves performance.

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::set_section_for_node): Declare new
overload.
(symtab_node::set_section_from_string): Rename from set_section.
(symtab_node::set_section_from_node): Declare.
* gcc/symtab.c (symtab_node::set_section_for_node): Define new
overload.
(symtab_node::set_section_from_string): Rename from set_section.
(symtab_node::set_section_from_node): Define.
(symtab_node::set_section): Call renamed set_section_from_string.
(symtab_node::set_section): Call new set_section_from_node.


diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 3b07258b31d..928a8bc2729 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -313,6 +313,10 @@ public:
      use set_section.  */
   void set_section_for_node (const char *section);

+  /* Like set_section_for_node, but copying the section name from another
+     node.  */
+  void set_section_for_node (const symtab_node &other);
+
   /* Set initialization priority to PRIORITY.  */
   void set_init_priority (priority_type priority);

@@ -627,8 +631,9 @@ protected:
                       void *data,
                       bool include_overwrite);
 private:
-  /* Worker for set_section.  */
-  static bool set_section (symtab_node *n, void *s);
+  /* Workers for set_section.  */
+  static bool set_section_from_string (symtab_node *n, void *s);
+  static bool set_section_from_node (symtab_node *n, void *o);

   /* Worker for symtab_resolve_alias.  */
   static bool set_implicit_section (symtab_node *n, void *);
diff --git a/gcc/symtab.c b/gcc/symtab.c
index a2aa519e760..40752addcb6 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -1596,15 +1596,37 @@ symtab_node::set_section_for_node (const char *section)
     }
 }

-/* Worker for set_section.  */
+void
+symtab_node::set_section_for_node (const symtab_node &other)
+{
+  if (x_section == other.x_section)
+    return;
+  if (get_section () && other.get_section ())
+    gcc_checking_assert (strcmp (get_section (), other.get_section ()) != 0);
+  release_section_hash_entry (x_section);
+  if (other.x_section)
+    x_section = retain_section_hash_entry (other.x_section);
+  else
+    x_section = NULL;
+}
+
+/* Workers for set_section.  */

 bool
-symtab_node::set_section (symtab_node *n, void *s)
+symtab_node::set_section_from_string (symtab_node *n, void *s)
 {
   n->set_section_for_node ((char *)s);
   return false;
 }

+bool
+symtab_node::set_section_from_node (symtab_node *n, void *o)
+{
+  const symtab_node &other = *static_cast<const symtab_node *> (o);
+  n->set_section_for_node (other);
+  return false;
+}
+
 /* Set section of symbol and its aliases.  */

 void
@@ -1612,15 +1634,14 @@ symtab_node::set_section (const char *section)
 {
   gcc_assert (!this->alias || !this->analyzed);
   call_for_symbol_and_aliases
-    (symtab_node::set_section, const_cast<char *>(section), true);
+    (symtab_node::set_section_from_string, 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);
+    (symtab_node::set_section_from_node, const_cast<symtab_node
*>(&other), true);
 }

 /* Return the initialization priority.  */

Re: [PATCH 1/3] Refactor copying decl section names

[Jeff Law]

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
> 

Re: [PATCH 1/3] Refactor copying decl section names

[Jeff Law]

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

Re: [PATCH 1/3] Refactor copying decl section names

[Jeff Law]

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 2/3] Refactor section name ref counting

[Jeff Law]

On 11/12/19 11:28 PM, Strager Neds wrote:
> symtab_node::set_section_for_node manages the reference count of
> section_hash_entry objects. I plan to add another function which needs
> to manage the reference count of these objects. To avoid duplicating
> code, factor the existing logic into reusable functions.
>
> 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/symtab.c (symtab_node::set_section_for_node): Extract reference
> counting logic into ...
> (retain_section_hash_entry): ... here (new function) and ...
> (release_section_hash_entry): ... here (new function).

I fixed some minor formatting problems, added function comments to the
factored-out code and re-tested this patch.  I'm pushing it to the trunk
momentarily.


jeff

Re: [PATCH 1/3] Refactor copying decl section names

[Alan Modra]

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 3/3] Improve efficiency of copying section from another tree

[Jeff Law]

On 11/12/19 11:29 PM, Strager Neds wrote:
> Several parts of GCC need to copy a section name from one tree (or
> symtab_node) to another. Currently, this is implemented naively:
>
> 1. Query the source's section name
> 2. Hash the section name string
> 3. Find the section_hash_entry in the symbol table
> 4. Increment the section_hash_entry's reference count
> 5. Assign the destination's section to the section_hash_entry
>
> Since we have the source's section_hash_entry, we can copy the section
> name from one symtab_node to another efficiently with the following
> algorithm:
>
> 1. Query the source's section_hash_entry
> 2. Increment the section_hash_entry's reference count
> 3. Assign the destination's section to the section_hash_entry
>
> Implement this algorithm in the overload of symtab_node::set_section
> which takes an existing symtab_node.
>
> I did not measure the performance impact of this patch. In
> particular, I do not know if this patch actually improves performance.
>
> 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::set_section_for_node): Declare new
> overload.
> (symtab_node::set_section_from_string): Rename from set_section.
> (symtab_node::set_section_from_node): Declare.
> * gcc/symtab.c (symtab_node::set_section_for_node): Define new
> overload.
> (symtab_node::set_section_from_string): Rename from set_section.
> (symtab_node::set_section_from_node): Define.
> (symtab_node::set_section): Call renamed set_section_from_string.
> (symtab_node::set_section): Call new set_section_from_node.

I've fixed up the ChangeLog, re-tested and pushed this patch to the trunk.

Thanks again for your patience,

jeff

Re: [PATCH 1/3] Refactor copying decl section names

[Jeff Law]

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

[Alan Modra]

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

[Jeff Law]

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 3/3] Improve efficiency of copying section from another tree

[Martin Liška]

On 11/13/19 7:29 AM, Strager Neds wrote:
> -/* Worker for set_section.  */
> +void
> +symtab_node::set_section_for_node (const symtab_node &other)
> +{
> +  if (x_section == other.x_section)
> +    return;
> +  if (get_section () && other.get_section ())
> +    gcc_checking_assert (strcmp (get_section (), other.get_section ()) != 0);
> +  release_section_hash_entry (x_section);
> +  if (other.x_section)
> +    x_section = retain_section_hash_entry (other.x_section);
> +  else
> +    x_section = NULL;
> +}
> +
> +/* Workers for set_section.  */
> 
>   bool
> -symtab_node::set_section (symtab_node *n, void *s)
> +symtab_node::set_section_from_string (symtab_node *n, void *s)
>   {
>     n->set_section_for_node ((char *)s);
>     return false;
>   }
> 
> +bool
> +symtab_node::set_section_from_node (symtab_node *n, void *o)
> +{
> +  const symtab_node &other = *static_cast<const symtab_node *> (o);
> +  n->set_section_for_node (other);
> +  return false;
> +}
> +
>   /* Set section of symbol and its aliases.  */

Hello.

Apparently, the patch caused the following regression:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98057

I've got a fix for it, but I would appreciate function comments
for the

void
symtab_node::set_section_for_node (const symtab_node &other)

and
bool
symtab_node::set_section_from_node (symtab_node *n, void *o)

Can you please add it?
Thanks,
Martin

Re: [PATCH 3/3] Improve efficiency of copying section from another tree

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]



On 11/30/20 5:16 AM, Martin Liška wrote:
> On 11/13/19 7:29 AM, Strager Neds wrote:
>> -/* Worker for set_section.  */
>> +void
>> +symtab_node::set_section_for_node (const symtab_node &other)
>> +{
>> +  if (x_section == other.x_section)
>> +    return;
>> +  if (get_section () && other.get_section ())
>> +    gcc_checking_assert (strcmp (get_section (), other.get_section
>> ()) != 0);
>> +  release_section_hash_entry (x_section);
>> +  if (other.x_section)
>> +    x_section = retain_section_hash_entry (other.x_section);
>> +  else
>> +    x_section = NULL;
>> +}
>> +
>> +/* Workers for set_section.  */
>>
>>   bool
>> -symtab_node::set_section (symtab_node *n, void *s)
>> +symtab_node::set_section_from_string (symtab_node *n, void *s)
>>   {
>>     n->set_section_for_node ((char *)s);
>>     return false;
>>   }
>>
>> +bool
>> +symtab_node::set_section_from_node (symtab_node *n, void *o)
>> +{
>> +  const symtab_node &other = *static_cast<const symtab_node *> (o);
>> +  n->set_section_for_node (other);
>> +  return false;
>> +}
>> +
>>   /* Set section of symbol and its aliases.  */
>
> Hello.
>
> Apparently, the patch caused the following regression:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98057
>
> I've got a fix for it, but I would appreciate function comments
> for the
>
> void
> symtab_node::set_section_for_node (const symtab_node &other)
>
> and
> bool
> symtab_node::set_section_from_node (symtab_node *n, void *o)
>
> Can you please add it?
I'll take care of it with the attached patch.

Jeff


[-- Attachment #2: P --]
[-- Type: text/plain, Size: 1007 bytes --]

commit dccae0f42e9e052b7721e805858d10d3ec345685
Author: Jeff Law <law@redhat.com>
Date:   Mon Nov 30 15:21:38 2020 -0700

    Add function comments for recently added member functions.
    
    gcc/
            * symtab.c (set_section_for_node): Add function comment.
            (set_section_from_node): Likewise.

diff --git a/gcc/symtab.c b/gcc/symtab.c
index 393d6b07870..fd7d553c112 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -1668,6 +1668,10 @@ symtab_node::set_section_for_node (const char *section)
     }
 }
 
+/* Set the section of node THIS to be the same as the section
+   of node OTHER.  Keep reference counts of the sections
+   up-to-date as needed.  */
+
 void
 symtab_node::set_section_for_node (const symtab_node &other)
 {
@@ -1691,6 +1695,9 @@ symtab_node::set_section_from_string (symtab_node *n, void *s)
   return false;
 }
 
+/* Set the section of node N to be the same as the section
+   of node O.  */
+
 bool
 symtab_node::set_section_from_node (symtab_node *n, void *o)
 {