On 2018-07-17 06:02 AM, Richard Biener wrote: > On Tue, Jul 17, 2018 at 8:10 AM Bernhard Reutner-Fischer > <rep.dot....@gmail.com> wrote: >> >> On 16 July 2018 21:38:36 CEST, Michael Ploujnikov >> <michael.ploujni...@oracle.com> wrote: >>> Hi, >>> >> >>> + clone_fn_ids = hash_map<const char *, unsigned>::create_ggc >>> (1000); >> >> Isn't 1000 a bit excessive? What about 64 or thereabouts? > > I'm not sure we should throw memory at this "problem" in this way. > What specific issue > does this address?
This goes along with the general theme of preventing changes to one function
affecting codegen of others. What I'm seeing in this case is when a function
bar is modified codegen decides to create more clones of it (eg: during the
constprop pass). These extra clones cause the global counter to increment so
the clones of the unchanged function foo are named differently only because of
a source change to bar. I was hoping that the testcase would be a good
illustration, but perhaps not; is there anything else I can do to make this
clearer?
>
> Iff then I belive forgoing the automatic counter addidion is the way
> to go and hand
> control of that to the callers (for example the caller in
> lto/lto-partition.c doesn't
> even seem to need that counter.
How can you tell that privatize_symbol_name_1 doesn't need the counter? I'm
assuming it has a good reason to call clone_function_name_1 rather than
appending ".lto_priv" itself.
> You also assume the string you key on persists - luckily the
> lto-partition.c caller
> leaks it but this makes your approach quite fragile in my eye (put the logic
> into clone_function_name instead, where you at least know you are dealing
> with a string from an indentifier which are never collected).
>
> Richard.
>
Is this what you had in mind?:
diff --git gcc/cgraphclones.c gcc/cgraphclones.c
index 6e84a31..f000420 100644
--- gcc/cgraphclones.c
+++ gcc/cgraphclones.c
@@ -512,7 +512,7 @@ cgraph_node::create_clone (tree new_decl, profile_count
prof_count,
return new_node;
}
-static GTY(()) unsigned int clone_fn_id_num;
+static GTY(()) hash_map<const char *, unsigned> *clone_fn_ids;
/* Return a new assembler name for a clone with SUFFIX of a decl named
NAME. */
@@ -521,14 +521,13 @@ tree
clone_function_name_1 (const char *name, const char *suffix)
{
size_t len = strlen (name);
- char *tmp_name, *prefix;
+ char *prefix;
prefix = XALLOCAVEC (char, len + strlen (suffix) + 2);
memcpy (prefix, name, len);
strcpy (prefix + len + 1, suffix);
prefix[len] = symbol_table::symbol_suffix_separator ();
- ASM_FORMAT_PRIVATE_NAME (tmp_name, prefix, clone_fn_id_num++);
- return get_identifier (tmp_name);
+ return get_identifier (prefix);
}
/* Return a new assembler name for a clone of DECL with SUFFIX. */
@@ -537,7 +536,17 @@ tree
clone_function_name (tree decl, const char *suffix)
{
tree name = DECL_ASSEMBLER_NAME (decl);
- return clone_function_name_1 (IDENTIFIER_POINTER (name), suffix);
+ char *decl_name = IDENTIFIER_POINTER (name);
+ char *numbered_name;
+ unsigned int *suffix_counter;
+ if (!clone_fn_ids) {
+ /* Initialize the per-function counter hash table if this is the first
call */
+ clone_fn_ids = hash_map<const char *, unsigned>::create_ggc (64);
+ }
+ suffix_counter = &clone_fn_ids->get_or_insert(name);
+ ASM_FORMAT_PRIVATE_NAME (numbered_name, decl_name, *suffix_counter);
+ *suffix_counter = *suffix_counter + 1;
+ return clone_function_name_1 (numbered_name, suffix);
}
- Michael
signature.asc
Description: OpenPGP digital signature
