Hi Martin, Thanks for the review.
On 2018-10-26 03:51 AM, Martin Liška wrote: > On 10/26/18 12:59 AM, Michael Ploujnikov wrote: >> I've taken the advice from a discussion on IRC and re-wrote the patch >> with more uniform function names and using overloading. >> >> I think this function accomplished the following goals: >> - remove clone numbering where it's not needed: >> final.c:final_scan_insn_1 and >> symtab.c:simd_symtab_node::noninterposable_alias. >> - name and document the clone naming API such that future users won't >> accidentally use the numbering when it's not necessary; if you need >> numbering then you need to explicitly ask for it with the right >> function >> - provide a new function that allows users to specify a clone number >> explicitly as an argument > > Hello. > > Thanks for reworking that. > >> >> My thoughts for future improvements: >> - It's a bit unfortunate that lto-partition.c:privatize_symbol_name_1 >> has to break the decl abstraction and pass in a string that it >> created into what I would consider the implementation-detail >> function. The best way I can think of to make it uniform with the >> rest of the users is to have it create a new empty decl with >> DECL_ASSEMBLER_NAME set to the new string > > That's not nice to create artificial declaration. Having string variant > is fine for me. Ok. > >> - It's unfortunate that I have to duplicate the separator >> concatenation in the numberless clone_function_name, but I think it >> has to be like that unless ASM_FORMAT_PRIVATE_NAME making the >> number optional. >> > > That's also fine for me. I'm attaching small nits that I found. > And please reformat following chunk in ChangeLog entry: > > * cgraph.h (clone_function_name_1): Replaced by new > clone_function_name_numbered that takes name as string; for > privatize_symbol_name_1 use only. (clone_function_name): > Renamed to clone_function_name_numbered to be explicit about > numbering. (clone_function_name): New two-argument function > that does not number its output. (clone_function_name): New > three-argument function that takes a number to append to its > output. > > into: > > * cgraph.h (clone_function_name_1): Replaced by new > clone_function_name_numbered that takes name as string; for > privatize_symbol_name_1 use only. > (clone_function_name): Renamed to clone_function_name_numbered > to be explicit about... Fixed, assuming you wanted me to start each function on a new line. > suffix, > - (char*)0)); > + NULL)); > return get_identifier (result); > } I've actually been told that NULL isn't always the same on some targets and that I should use (char*)0 instead. Note that libiberty/concat.c itself uses (char*)0. > > I'm adding Honza to CC, hope he can review it quickly. > > Thanks, > Martin > Thanks again, Michael
From aea94273e7a477a03d1ee10a5d9043d6d13b8e8d Mon Sep 17 00:00:00 2001
From: Michael Ploujnikov <michael.ploujni...@oracle.com>
Date: Thu, 25 Oct 2018 13:16:36 -0400
Subject: [PATCH] Avoid unnecessarily numbering cloned symbols.
gcc/ChangeLog:
2018-10-26 Michael Ploujnikov <michael.ploujni...@oracle.com>
* cgraph.h (clone_function_name_1): Replaced by new
clone_function_name_numbered that takes name as string; for
privatize_symbol_name_1 use only.
(clone_function_name): Renamed to
clone_function_name_numbered to be explicit about numbering.
(clone_function_name): New two-argument function that does
not number its output.
(clone_function_name): New three-argument function that
takes a number to append to its output.
* cgraphclones.c (duplicate_thunk_for_node):
(clone_function_name_1): Renamed.
(clone_function_name_numbered): Two new functions.
(clone_function_name): Improved documentation.
(cgraph_node::create_virtual_clone): Use clone_function_name_numbered.
* config/rs6000/rs6000.c (make_resolver_func): Ditto.
* final.c (final_scan_insn_1): Use the new clone_function_name
without numbering.
* multiple_target.c (create_dispatcher_calls): Ditto.
(create_target_clone): Ditto.
* omp-expand.c (grid_expand_target_grid_body): Ditto.
* omp-low.c (create_omp_child_function_name): Ditto.
* omp-simd-clone.c (simd_clone_create): Ditto.
* symtab.c (simd_symtab_node::noninterposable_alias): Use the
new clone_function_name without numbering.
gcc/lto/ChangeLog:
2018-10-26 Michael Ploujnikov <michael.ploujni...@oracle.com>
* lto-partition.c (privatize_symbol_name_1): Use
clone_function_name_numbered.
gcc/testsuite/ChangeLog:
2018-10-26 Michael Ploujnikov <michael.ploujni...@oracle.com>
* gcc.dg/tree-prof/cold_partition_label.c: Update for cold
section names without numbers.
* gcc.dg/tree-prof/section-attr-1.c: Ditto.
* gcc.dg/tree-prof/section-attr-2.c: Ditto.
* gcc.dg/tree-prof/section-attr-3.c: Ditto.
---
gcc/cgraph.h | 7 ++-
gcc/cgraphclones.c | 69 ++++++++++++++++++----
gcc/config/rs6000/rs6000.c | 2 +-
gcc/lto/lto-partition.c | 4 +-
gcc/multiple_target.c | 8 +--
gcc/omp-expand.c | 3 +-
gcc/omp-low.c | 4 +-
gcc/omp-simd-clone.c | 3 +-
.../gcc.dg/tree-prof/cold_partition_label.c | 4 +-
gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c | 2 +-
gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c | 2 +-
gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c | 2 +-
12 files changed, 82 insertions(+), 28 deletions(-)
diff --git gcc/cgraph.h gcc/cgraph.h
index a8b1b4c..971065d 100644
--- gcc/cgraph.h
+++ gcc/cgraph.h
@@ -2368,8 +2368,11 @@ basic_block init_lowered_empty_function (tree, bool, profile_count);
tree thunk_adjust (gimple_stmt_iterator *, tree, bool, HOST_WIDE_INT, tree);
/* In cgraphclones.c */
-tree clone_function_name_1 (const char *, const char *);
-tree clone_function_name (tree decl, const char *);
+tree clone_function_name_numbered (const char *name, const char *suffix);
+tree clone_function_name_numbered (tree decl, const char *suffix);
+tree clone_function_name (const char *name, const char *suffix,
+ unsigned long number);
+tree clone_function_name (tree decl, const char *suffix);
void tree_function_versioning (tree, tree, vec<ipa_replace_map *, va_gc> *,
bool, bitmap, bool, bitmap, basic_block);
diff --git gcc/cgraphclones.c gcc/cgraphclones.c
index 6e84a31..fd2967c 100644
--- gcc/cgraphclones.c
+++ gcc/cgraphclones.c
@@ -316,7 +316,8 @@ duplicate_thunk_for_node (cgraph_node *thunk, cgraph_node *node)
gcc_checking_assert (!DECL_RESULT (new_decl));
gcc_checking_assert (!DECL_RTL_SET_P (new_decl));
- DECL_NAME (new_decl) = clone_function_name (thunk->decl, "artificial_thunk");
+ DECL_NAME (new_decl) = clone_function_name_numbered (thunk->decl,
+ "artificial_thunk");
SET_DECL_ASSEMBLER_NAME (new_decl, DECL_NAME (new_decl));
new_thunk = cgraph_node::create (new_decl);
@@ -514,11 +515,41 @@ cgraph_node::create_clone (tree new_decl, profile_count prof_count,
static GTY(()) unsigned int clone_fn_id_num;
-/* Return a new assembler name for a clone with SUFFIX of a decl named
- NAME. */
+/* Return a new assembler name for a clone of decl named NAME. Apart
+ from the string SUFFIX, the new name will end with a unique (for
+ each NAME) unspecified number. If clone numbering is not needed
+ then the two argument clone_function_name should be used instead.
+ Should not be called directly except for by
+ lto-partition.c:privatize_symbol_name_1. */
tree
-clone_function_name_1 (const char *name, const char *suffix)
+clone_function_name_numbered (const char *name, const char *suffix)
+{
+ return clone_function_name (name, suffix, clone_fn_id_num++);
+}
+
+/* Return a new assembler name for a clone of DECL. Apart from string
+ SUFFIX, the new name will end with a unique (for each DECL
+ assembler name) unspecified number. If clone numbering is not
+ needed then the two argument clone_function_name should be used
+ instead. */
+
+tree
+clone_function_name_numbered (tree decl, const char *suffix)
+{
+ tree name = DECL_ASSEMBLER_NAME (decl);
+ return clone_function_name_numbered (IDENTIFIER_POINTER (name),
+ suffix);
+}
+
+/* Return a new assembler name for a clone of decl named NAME. Apart
+ from the string SUFFIX, the new name will end with the specified
+ NUMBER. If clone numbering is not needed then the two argument
+ clone_function_name should be used instead. */
+
+tree
+clone_function_name (const char *name, const char *suffix,
+ unsigned long number)
{
size_t len = strlen (name);
char *tmp_name, *prefix;
@@ -527,17 +558,34 @@ clone_function_name_1 (const char *name, const char *suffix)
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++);
+ ASM_FORMAT_PRIVATE_NAME (tmp_name, prefix, number);
return get_identifier (tmp_name);
}
-/* Return a new assembler name for a clone of DECL with SUFFIX. */
+/* Return a new assembler name ending with the string SUFFIX for a
+ clone of DECL. */
tree
clone_function_name (tree decl, const char *suffix)
{
- tree name = DECL_ASSEMBLER_NAME (decl);
- return clone_function_name_1 (IDENTIFIER_POINTER (name), suffix);
+ tree identifier = DECL_ASSEMBLER_NAME (decl);
+ /* For consistency this needs to behave the same way as
+ ASM_FORMAT_PRIVATE_NAME does, but without the final number
+ suffix. */
+ char *separator = XALLOCAVEC (char, 2);
+ separator[0] = symbol_table::symbol_suffix_separator ();
+ separator[1] = 0;
+#if defined (NO_DOT_IN_LABEL) && defined (NO_DOLLAR_IN_LABEL)
+ const char *prefix = "__";
+#else
+ const char *prefix = "";
+#endif
+ char *result = ACONCAT ((prefix,
+ IDENTIFIER_POINTER (identifier),
+ separator,
+ suffix,
+ (char*)0));
+ return get_identifier (result);
}
@@ -585,7 +633,8 @@ cgraph_node::create_virtual_clone (vec<cgraph_edge *> redirect_callers,
strcpy (name + len + 1, suffix);
name[len] = '.';
DECL_NAME (new_decl) = get_identifier (name);
- SET_DECL_ASSEMBLER_NAME (new_decl, clone_function_name (old_decl, suffix));
+ SET_DECL_ASSEMBLER_NAME (new_decl, clone_function_name_numbered (old_decl,
+ suffix));
SET_DECL_RTL (new_decl, NULL);
new_node = create_clone (new_decl, count, false,
@@ -964,7 +1013,7 @@ cgraph_node::create_version_clone_with_body
= build_function_decl_skip_args (old_decl, args_to_skip, skip_return);
/* Generate a new name for the new version. */
- DECL_NAME (new_decl) = clone_function_name (old_decl, suffix);
+ DECL_NAME (new_decl) = clone_function_name_numbered (old_decl, suffix);
SET_DECL_ASSEMBLER_NAME (new_decl, DECL_NAME (new_decl));
SET_DECL_RTL (new_decl, NULL);
diff --git gcc/config/rs6000/rs6000.c gcc/config/rs6000/rs6000.c
index c2af4b8..c314739 100644
--- gcc/config/rs6000/rs6000.c
+++ gcc/config/rs6000/rs6000.c
@@ -36512,7 +36512,7 @@ make_resolver_func (const tree default_decl,
{
/* Make the resolver function static. The resolver function returns
void *. */
- tree decl_name = clone_function_name (default_decl, "resolver");
+ tree decl_name = clone_function_name_numbered (default_decl, "resolver");
const char *resolver_name = IDENTIFIER_POINTER (decl_name);
tree type = build_function_type_list (ptr_type_node, NULL_TREE);
tree decl = build_fn_decl (resolver_name, type);
diff --git gcc/lto/lto-partition.c gcc/lto/lto-partition.c
index c7a5710..24e7c23 100644
--- gcc/lto/lto-partition.c
+++ gcc/lto/lto-partition.c
@@ -964,8 +964,8 @@ privatize_symbol_name_1 (symtab_node *node, tree decl)
name = maybe_rewrite_identifier (name);
symtab->change_decl_assembler_name (decl,
- clone_function_name_1 (name,
- "lto_priv"));
+ clone_function_name_numbered (
+ name, "lto_priv"));
if (node->lto_file_data)
lto_record_renamed_decl (node->lto_file_data, name,
diff --git gcc/multiple_target.c gcc/multiple_target.c
index a1fe09a..9bbfeee 100644
--- gcc/multiple_target.c
+++ gcc/multiple_target.c
@@ -162,8 +162,8 @@ create_dispatcher_calls (struct cgraph_node *node)
}
symtab->change_decl_assembler_name (node->decl,
- clone_function_name (node->decl,
- "default"));
+ clone_function_name_numbered (
+ node->decl, "default"));
/* FIXME: copy of cgraph_node::make_local that should be cleaned up
in next stage1. */
@@ -312,8 +312,8 @@ create_target_clone (cgraph_node *node, bool definition, char *name)
new_node = cgraph_node::get_create (new_decl);
/* Generate a new name for the new version. */
symtab->change_decl_assembler_name (new_node->decl,
- clone_function_name (node->decl,
- name));
+ clone_function_name_numbered (
+ node->decl, name));
}
return new_node;
}
diff --git gcc/omp-expand.c gcc/omp-expand.c
index d2a77c0..faff02b 100644
--- gcc/omp-expand.c
+++ gcc/omp-expand.c
@@ -7625,7 +7625,8 @@ grid_expand_target_grid_body (struct omp_region *target)
expand_omp (gpukernel->inner);
tree kern_fndecl = copy_node (orig_child_fndecl);
- DECL_NAME (kern_fndecl) = clone_function_name (kern_fndecl, "kernel");
+ DECL_NAME (kern_fndecl) = clone_function_name_numbered (kern_fndecl,
+ "kernel");
SET_DECL_ASSEMBLER_NAME (kern_fndecl, DECL_NAME (kern_fndecl));
tree tgtblock = gimple_block (tgt_stmt);
tree fniniblock = make_node (BLOCK);
diff --git gcc/omp-low.c gcc/omp-low.c
index 843c66f..29539b3 100644
--- gcc/omp-low.c
+++ gcc/omp-low.c
@@ -1531,8 +1531,8 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
static tree
create_omp_child_function_name (bool task_copy)
{
- return clone_function_name (current_function_decl,
- task_copy ? "_omp_cpyfn" : "_omp_fn");
+ return clone_function_name_numbered (current_function_decl,
+ task_copy ? "_omp_cpyfn" : "_omp_fn");
}
/* Return true if CTX may belong to offloaded code: either if current function
diff --git gcc/omp-simd-clone.c gcc/omp-simd-clone.c
index b15adf0..7bafe39 100644
--- gcc/omp-simd-clone.c
+++ gcc/omp-simd-clone.c
@@ -444,7 +444,8 @@ simd_clone_create (struct cgraph_node *old_node)
{
tree old_decl = old_node->decl;
tree new_decl = copy_node (old_node->decl);
- DECL_NAME (new_decl) = clone_function_name (old_decl, "simdclone");
+ DECL_NAME (new_decl) = clone_function_name_numbered (old_decl,
+ "simdclone");
SET_DECL_ASSEMBLER_NAME (new_decl, DECL_NAME (new_decl));
SET_DECL_RTL (new_decl, NULL);
DECL_STATIC_CONSTRUCTOR (new_decl) = 0;
diff --git gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
index 52518cf..450308d 100644
--- gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
+++ gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
@@ -37,6 +37,6 @@ main (int argc, char *argv[])
return 0;
}
-/* { dg-final-use { scan-assembler "foo\[._\]+cold\[\._\]+0" { target *-*-linux* *-*-gnu* } } } */
-/* { dg-final-use { scan-assembler "size\[ \ta-zA-Z0-0\]+foo\[._\]+cold\[\._\]+0" { target *-*-linux* *-*-gnu* } } } */
+/* { dg-final-use { scan-assembler "foo\[._\]+cold" { target *-*-linux* *-*-gnu* } } } */
+/* { dg-final-use { scan-assembler "size\[ \ta-zA-Z0-0\]+foo\[._\]+cold" { target *-*-linux* *-*-gnu* } } } */
/* { dg-final-use { scan-tree-dump-not "Invalid sum" "optimized"} } */
diff --git gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c
index ee6662e..1bb8cd9 100644
--- gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c
+++ gcc/testsuite/gcc.dg/tree-prof/section-attr-1.c
@@ -42,4 +42,4 @@ foo (int path)
}
}
-/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
+/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold" { target *-*-linux* *-*-gnu* } } } */
diff --git gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c
index 898a395..31be7eb 100644
--- gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c
+++ gcc/testsuite/gcc.dg/tree-prof/section-attr-2.c
@@ -41,4 +41,4 @@ foo (int path)
}
}
-/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
+/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold" { target *-*-linux* *-*-gnu* } } } */
diff --git gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c
index 36829dc..0e64001 100644
--- gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c
+++ gcc/testsuite/gcc.dg/tree-prof/section-attr-3.c
@@ -42,4 +42,4 @@ foo (int path)
}
}
-/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold\.0" { target *-*-linux* *-*-gnu* } } } */
+/* { dg-final-use { scan-assembler "\.section\[\t \]*\.text\.unlikely\[\\n\\r\]+\[\t \]*\.size\[\t \]*foo\.cold" { target *-*-linux* *-*-gnu* } } } */
--
2.7.4
signature.asc
Description: OpenPGP digital signature
