This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Make function clone name numbering independent.


On 2018-07-17 06:02 AM, Richard Biener wrote:
> On Tue, Jul 17, 2018 at 8:10 AM Bernhard Reutner-Fischer
> <rep.dot.nop@gmail.com> wrote:
>>
>> On 16 July 2018 21:38:36 CEST, Michael Ploujnikov <michael.ploujnikov@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



Attachment: signature.asc
Description: OpenPGP digital signature


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]