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

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Jul 20, 2018 at 4:48 AM Michael Ploujnikov
<michael.ploujnikov@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.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)

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
>