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

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Nov 28, 2018 at 10:09 PM Michael Ploujnikov
<michael.ploujnikov@oracle.com> wrote:
>
> Continuing from https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00228.html
>
> It took longer than expected, but I've finally rebased this on top of
> the new clone_function_name* API and included the requested
> optimizations.
>
> There are a few remaining spots where we could probably apply similar
> optimizations:
>
> - gcc/multiple_target.c:create_target_clone
> - gcc/multiple_target.c:create_dispatcher_calls
> - gcc/omp-expand.c:grid_expand_target_grid_body
>
> But I've yet to figure out how these work in detail and with the new
> API these shouldn't block the main change from being merged.
>
> I've also included a small change to rs6000 which I'm pretty sure is
> safe, but I have no way of testing.
>
> I'm not sure what's the consensus on what's more appropriate, but I
> thought that it would be a good idea to keep these changes as separate
> patches since only the first one really changes behaviour, while the
> rest are optimizations. It's conceivable that someone who is
> backporting this to an older release might want to just backport the
> core bits of the change. I can re-submit it as one patch if that's
> more appropriate.
>
> Everything in these patches was bootstrapped and regression tested on
> x86_64.
>
> Ok for trunk?

+  unsigned &clone_number = lto_clone_numbers->get_or_insert (
+     IDENTIFIER_POINTER (DECL_NAME (decl)));
   name = maybe_rewrite_identifier (name);
   symtab->change_decl_assembler_name (decl,
-      clone_function_name_numbered (
-  name, "lto_priv"));
+      clone_function_name (
+  name, "lto_priv", clone_number));

since we use 'name' from maybe_rewrite_identifier in the end wouldn't it
make more sense to use that as a key for lto_clone_numbers?

For the ipa-hsa.c changes since we iterate over all defined functions
we at most create a single clone per cgraph_node.  That means your
hash-map keyed on cgraph_node is useless and you could use
an unnumbered clone (or a static zero number).

-  SET_DECL_ASSEMBLER_NAME (new_decl, clone_function_name_numbered (old_decl,
-   suffix));
+  SET_DECL_ASSEMBLER_NAME (new_decl,
+   clone_function_name (
+     IDENTIFIER_POINTER (
+       DECL_ASSEMBLER_NAME (old_decl)),
+     suffix,
+     num_suffix));

can you please hide the implementation detail of accessing the assembler name
by adding an overload to clone_function_name_numbered with an explicit
number?

OK with those changes.

Thanks,
Richard.



>
> - Michael