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....@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
>
>
>
Ping, and below is the updated version of the full patch with changelogs:
gcc:
2018-07-16 Michael Ploujnikov <michael.ploujni...@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.ploujni...@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
signature.asc
Description: OpenPGP digital signature
