Re: [PATCH 3/3] Improve efficiency of copying section from another tree
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 (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
commit dccae0f42e9e052b7721e805858d10d3ec345685
Author: Jeff Law
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)
{
Re: [PATCH 3/3] Improve efficiency of copying section from another tree
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 (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
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 > > * 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
[PATCH 3/3] Improve efficiency of copying section from another tree
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
* 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 (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(section), true);
+(symtab_node::set_section_from_string, const_cast(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(section), true);
+(symtab_node::set_section_from_node, const_cast(&other), true);
}
/* Return the initialization priority. */
