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]

Re: [PATCH] Make function clone name numbering independent.


On 2018-07-17 04:25 PM, Michael Ploujnikov wrote:
> 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
> 
> 
> 

Ping, and below is the updated version of the full patch with changelogs:


gcc:
2018-07-16  Michael Ploujnikov  <michael.ploujnikov@oracle.com>

       Make function clone name numbering independent.
       * cgraphclones.c: Replace clone_fn_id_num with clone_fn_ids.
       (clone_function_name_1): Move suffixing to clone_function_name
       and change it to use per-function clone_fn_ids.

testsuite:
2018-07-16  Michael Ploujnikov  <michael.ploujnikov@oracle.com>

	Clone id counters should be completely independent from one another.
	* gcc/testsuite/gcc.dg/independent-cloneids-1.c: New test.

---
 gcc/cgraphclones.c                            | 19 ++++++++++----
 gcc/testsuite/gcc.dg/independent-cloneids-1.c | 38 +++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/independent-cloneids-1.c

diff --git gcc/cgraphclones.c gcc/cgraphclones.c
index 6e84a31..e1a77a2 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);
+  const 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(decl_name);
+  ASM_FORMAT_PRIVATE_NAME (numbered_name, decl_name, *suffix_counter);
+  *suffix_counter = *suffix_counter + 1;
+  return clone_function_name_1 (numbered_name, suffix);
 }
 
 
diff --git gcc/testsuite/gcc.dg/independent-cloneids-1.c gcc/testsuite/gcc.dg/independent-cloneids-1.c
new file mode 100644
index 0000000..d723e20
--- /dev/null
+++ gcc/testsuite/gcc.dg/independent-cloneids-1.c
@@ -0,0 +1,38 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fipa-cp -fipa-cp-clone -fdump-ipa-cp"  } */
+
+extern int printf (const char *, ...);
+
+static int __attribute__ ((noinline))
+foo (int arg)
+{
+  return 7 * arg;
+}
+
+static int __attribute__ ((noinline))
+bar (int arg)
+{
+  return arg * arg;
+}
+
+int
+baz (int arg)
+{
+  printf("%d\n", bar (3));
+  printf("%d\n", bar (4));
+  printf("%d\n", foo (5));
+  printf("%d\n", foo (6));
+  /* adding or removing the following call should not affect foo
+     function's clone numbering */
+  printf("%d\n", bar (7));
+  return foo (8);
+}
+
+/* { dg-final { scan-ipa-dump "Function bar.constprop.0" "cp" } } */
+/* { dg-final { scan-ipa-dump "Function bar.constprop.1" "cp" } } */
+/* { dg-final { scan-ipa-dump "Function bar.constprop.3" "cp" } } */
+/* { dg-final { scan-ipa-dump "Function foo.constprop.0" "cp" } } */
+/* { dg-final { scan-ipa-dump "Function foo.constprop.1" "cp" } } */
+/* { dg-final { scan-ipa-dump "Function foo.constprop.2" "cp" } } */
+/* { dg-final { scan-ipa-dump-not "Function foo.constprop.3" "cp" } } */
+/* { dg-final { scan-ipa-dump-not "Function foo.constprop.4" "cp" } } */
-- 
2.7.4

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]