On Fri, Jul 20, 2018 at 4:48 AM Michael Ploujnikov
<michael.ploujni...@oracle.com> wrote:
>
> 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)
pass this function the counter to use....
> {
> 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++);
and keep using ASM_FORMAT_PRIVATE_NAME here. You need to change
the lto/lto-partition.c caller (just use zero as counter).
> - 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);
> + }
I still do not like throwing memory at the problem in this way for the
little benefit
this change provides :/
So no approval from me at this point...
Richard.
> + 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
>
