From: Michael Ploujnikov <michael.ploujnikov@oracle.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: Martin Jambor <mjambor@suse.cz>,
GCC Patches <gcc-patches@gcc.gnu.org>,
Jan Hubicka <hubicka@ucw.cz>,
Segher Boessenkool <segher@kernel.crashing.org>
Subject: Re: [PATCH v3] Make function clone name numbering independent.
Date: Fri, 30 Nov 2018 21:11:00 -0000 [thread overview]
Message-ID: <fa12f39f-6d95-c3ab-39f9-06481239e234@oracle.com> (raw)
In-Reply-To: <CAFiYyc0RqvSYCzBpHYwufN4DKMSf9au_NFj2pGjqic7WCefe-A@mail.gmail.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 1570 bytes --]
Hi,
On 2018-11-30 2:25 a.m., Richard Biener wrote:
> + 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?
Yup, that looks much better. Fixed it.
> 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).
Good catch. I was thinking of doing this, but it somehow fell through the cracks during the rebase.
>
> - 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?
Done.
>
> OK with those changes.
>
> Thanks,
> Richard.
Thank you for the review. I will commit as soon as my last test run finishes.
- Michael
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 0001-Make-function-assembly-more-independent.patch --]
[-- Type: text/x-patch; name="0001-Make-function-assembly-more-independent.patch", Size: 3629 bytes --]
From ac1f1579d37804c97833d460ec6cd5b87d6184c7 Mon Sep 17 00:00:00 2001
From: Michael Ploujnikov <michael.ploujnikov@oracle.com>
Date: Thu, 1 Nov 2018 12:57:30 -0400
Subject: [PATCH 1/3] Make function assembly more independent.
This is achieved by having clone_function_name assign unique clone
numbers for each function independently.
gcc:
2018-11-30 Michael Ploujnikov <michael.ploujnikov@oracle.com>
* cgraphclones.c: Replaced clone_fn_id_num with clone_fn_ids;
hash map.
(clone_function_name_numbered): Use clone_fn_ids.
gcc/testsuite:
2018-11-30 Michael Ploujnikov <michael.ploujnikov@oracle.com>
* gcc.dg/independent-cloneids-1.c: New test.
---
gcc/cgraphclones.c | 10 ++++-
gcc/testsuite/gcc.dg/independent-cloneids-1.c | 38 +++++++++++++++++++
2 files changed, 46 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/independent-cloneids-1.c
diff --git gcc/cgraphclones.c gcc/cgraphclones.c
index e17959c0ca..fdd84b60d3 100644
--- gcc/cgraphclones.c
+++ gcc/cgraphclones.c
@@ -513,7 +513,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 of decl named NAME. Apart
from the string SUFFIX, the new name will end with a unique (for
@@ -525,7 +525,13 @@ static GTY(()) unsigned int clone_fn_id_num;
tree
clone_function_name_numbered (const char *name, const char *suffix)
{
- return clone_function_name (name, suffix, clone_fn_id_num++);
+ /* Initialize the function->counter mapping the first time it's
+ needed. */
+ if (!clone_fn_ids)
+ clone_fn_ids = hash_map<const char *, unsigned int>::create_ggc (64);
+ unsigned int &suffix_counter = clone_fn_ids->get_or_insert (
+ IDENTIFIER_POINTER (get_identifier (name)));
+ return clone_function_name (name, suffix, suffix_counter++);
}
/* Return a new assembler name for a clone of DECL. Apart from string
diff --git gcc/testsuite/gcc.dg/independent-cloneids-1.c gcc/testsuite/gcc.dg/independent-cloneids-1.c
new file mode 100644
index 0000000000..3203895492
--- /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" } */
+
+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-assembler-times {(?n)\m_*bar[.$_]constprop[.$_]0:} 1 } } */
+/* { dg-final { scan-assembler-times {(?n)\m_*bar[.$_]constprop[.$_]1:} 1 } } */
+/* { dg-final { scan-assembler-times {(?n)\m_*bar[.$_]constprop[.$_]2:} 1 } } */
+/* { dg-final { scan-assembler-times {(?n)\m_*foo[.$_]constprop[.$_]0:} 1 } } */
+/* { dg-final { scan-assembler-times {(?n)\m_*foo[.$_]constprop[.$_]1:} 1 } } */
+/* { dg-final { scan-assembler-times {(?n)\m_*foo[.$_]constprop[.$_]2:} 1 } } */
+/* { dg-final { scan-assembler-not {(?n)\m_*foo[.$_]constprop[.$_]3:} } } */
+/* { dg-final { scan-assembler-not {(?n)\m_*foo[.$_]constprop[.$_]4:} } } */
--
2.19.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.3: 0002-Minimize-clone-counter-memory-usage-in-create_virtua.patch --]
[-- Type: text/x-patch; name="0002-Minimize-clone-counter-memory-usage-in-create_virtua.patch", Size: 7198 bytes --]
From 9656fc66b05c4ee9efd1b4a0533d564a35a85bc5 Mon Sep 17 00:00:00 2001
From: Michael Ploujnikov <michael.ploujnikov@oracle.com>
Date: Mon, 17 Sep 2018 16:02:27 -0400
Subject: [PATCH 2/3] Minimize clone counter memory usage in
create_virtual_clone.
Based on Martin Jambour's suggestion:
https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00111.html
gcc:
2018-11-30 Michael Ploujnikov <michael.ploujnikov@oracle.com>
* cgraph.h (clone_function_name): Add a variant that takes a
tree decl.
* cgraph.h (cgraph_node::create_virtual_clone): Add a new
argument: num_suffix.
* cgraphclones.c (cgraph_node::create_virtual_clone): Pass
num_suffix to clone_function_name.
(clone_function_name): Add a variant that takes a tree decl.
* ipa-cp.c (create_specialized_node): Keep track of clone
counters in clone_num_suffixes hash map.
(ipcp_driver): Free the counter hash map.
* ipa-hsa.c (process_hsa_functions): Creates at most one hsa
clone per function.
---
gcc/cgraph.h | 10 +++++++---
gcc/cgraphclones.c | 25 ++++++++++++++++++++-----
gcc/ipa-cp.c | 10 +++++++++-
gcc/ipa-hsa.c | 4 ++--
4 files changed, 38 insertions(+), 11 deletions(-)
diff --git gcc/cgraph.h gcc/cgraph.h
index c13d79850f..689bb828ca 100644
--- gcc/cgraph.h
+++ gcc/cgraph.h
@@ -968,11 +968,13 @@ public:
cgraph_node *new_inlined_to,
bitmap args_to_skip, const char *suffix = NULL);
- /* Create callgraph node clone with new declaration. The actual body will
- be copied later at compilation stage. */
+ /* Create callgraph node clone with new declaration. The actual body will be
+ copied later at compilation stage. The name of the new clone will be
+ constructed from the name of the original node, SUFFIX and NUM_SUFFIX. */
cgraph_node *create_virtual_clone (vec<cgraph_edge *> redirect_callers,
vec<ipa_replace_map *, va_gc> *tree_map,
- bitmap args_to_skip, const char * suffix);
+ bitmap args_to_skip, const char * suffix,
+ unsigned num_suffix);
/* cgraph node being removed from symbol table; see if its entry can be
replaced by other inline clone. */
@@ -2386,6 +2388,8 @@ 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,
+ unsigned long number);
tree clone_function_name (tree decl, const char *suffix);
void tree_function_versioning (tree, tree, vec<ipa_replace_map *, va_gc> *,
diff --git gcc/cgraphclones.c gcc/cgraphclones.c
index fdd84b60d3..2b7598e29e 100644
--- gcc/cgraphclones.c
+++ gcc/cgraphclones.c
@@ -568,6 +568,19 @@ clone_function_name (const char *name, const char *suffix,
return get_identifier (tmp_name);
}
+/* Return a new assembler name for a clone of DECL. 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 (tree decl, const char *suffix,
+ unsigned long number)
+{
+ return clone_function_name (
+ IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)), suffix, number);
+}
+
/* Return a new assembler name ending with the string SUFFIX for a
clone of DECL. */
@@ -595,8 +608,9 @@ clone_function_name (tree decl, const char *suffix)
}
-/* Create callgraph node clone with new declaration. The actual body will
- be copied later at compilation stage.
+/* Create callgraph node clone with new declaration. The actual body will be
+ copied later at compilation stage. The name of the new clone will be
+ constructed from the name of the original node, SUFFIX and NUM_SUFFIX.
TODO: after merging in ipa-sra use function call notes instead of args_to_skip
bitmap interface.
@@ -604,7 +618,8 @@ clone_function_name (tree decl, const char *suffix)
cgraph_node *
cgraph_node::create_virtual_clone (vec<cgraph_edge *> redirect_callers,
vec<ipa_replace_map *, va_gc> *tree_map,
- bitmap args_to_skip, const char * suffix)
+ bitmap args_to_skip, const char * suffix,
+ unsigned num_suffix)
{
tree old_decl = decl;
cgraph_node *new_node = NULL;
@@ -639,8 +654,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_numbered (old_decl,
- suffix));
+ SET_DECL_ASSEMBLER_NAME (new_decl,
+ clone_function_name (old_decl, suffix, num_suffix));
SET_DECL_RTL (new_decl, NULL);
new_node = create_clone (new_decl, count, false,
diff --git gcc/ipa-cp.c gcc/ipa-cp.c
index 4471bae11c..e0cd1bc45b 100644
--- gcc/ipa-cp.c
+++ gcc/ipa-cp.c
@@ -376,6 +376,9 @@ static profile_count max_count;
static long overall_size, max_new_size;
+/* Node to unique clone suffix number map. */
+static hash_map<cgraph_node *, unsigned> *clone_num_suffixes;
+
/* Return the param lattices structure corresponding to the Ith formal
parameter of the function described by INFO. */
static inline struct ipcp_param_lattices *
@@ -3828,8 +3831,11 @@ create_specialized_node (struct cgraph_node *node,
}
}
+ unsigned &suffix_counter = clone_num_suffixes->get_or_insert (node);
new_node = node->create_virtual_clone (callers, replace_trees,
- args_to_skip, "constprop");
+ args_to_skip, "constprop",
+ suffix_counter);
+ suffix_counter++;
bool have_self_recursive_calls = !self_recursive_calls.is_empty ();
for (unsigned j = 0; j < self_recursive_calls.length (); j++)
@@ -5043,6 +5049,7 @@ ipcp_driver (void)
ipa_check_create_node_params ();
ipa_check_create_edge_args ();
+ clone_num_suffixes = new hash_map<cgraph_node *, unsigned>;
if (dump_file)
{
@@ -5064,6 +5071,7 @@ ipcp_driver (void)
ipcp_store_vr_results ();
/* Free all IPCP structures. */
+ delete clone_num_suffixes;
free_toporder_info (&topo);
delete edge_clone_summaries;
edge_clone_summaries = NULL;
diff --git gcc/ipa-hsa.c gcc/ipa-hsa.c
index 7c6ceaab8f..63b41eabc9 100644
--- gcc/ipa-hsa.c
+++ gcc/ipa-hsa.c
@@ -88,7 +88,7 @@ process_hsa_functions (void)
continue;
cgraph_node *clone
= node->create_virtual_clone (vec <cgraph_edge *> (),
- NULL, NULL, "hsa");
+ NULL, NULL, "hsa", 0);
TREE_PUBLIC (clone->decl) = TREE_PUBLIC (node->decl);
clone->externally_visible = node->externally_visible;
@@ -109,7 +109,7 @@ process_hsa_functions (void)
continue;
cgraph_node *clone
= node->create_virtual_clone (vec <cgraph_edge *> (),
- NULL, NULL, "hsa");
+ NULL, NULL, "hsa", 0);
TREE_PUBLIC (clone->decl) = TREE_PUBLIC (node->decl);
clone->externally_visible = node->externally_visible;
--
2.19.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.4: 0003-Minimize-clone-counter-memory-usage-in-LTO.patch --]
[-- Type: text/x-patch; name="0003-Minimize-clone-counter-memory-usage-in-LTO.patch", Size: 2525 bytes --]
From 15283936692d1f0a8ff72c3e3211bb72eb5d11f8 Mon Sep 17 00:00:00 2001
From: Michael Ploujnikov <michael.ploujnikov@oracle.com>
Date: Mon, 26 Nov 2018 14:22:39 -0500
Subject: [PATCH 3/3] Minimize clone counter memory usage in LTO.
gcc/lto:
2018-11-30 Michael Ploujnikov <michael.ploujnikov@oracle.com>
* lto-partition.c (privatize_symbol_name_1): Keep track of
non-unique symbol counters in the lto_clone_numbers hash
map.
(lto_promote_cross_file_statics): Allocate and free the
lto_clone_numbers hash map.
(lto_promote_statics_nonwpa): Free the lto_clone_numbers hash
map.
---
gcc/lto/lto-partition.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git gcc/lto/lto-partition.c gcc/lto/lto-partition.c
index 24e7c23859..867f075b58 100644
--- gcc/lto/lto-partition.c
+++ gcc/lto/lto-partition.c
@@ -951,6 +951,9 @@ validize_symbol_for_target (symtab_node *node)
}
}
+/* Maps symbol names to unique lto clone counters. */
+static hash_map<const char *, unsigned> *lto_clone_numbers;
+
/* Helper for privatize_symbol_name. Mangle NODE symbol name
represented by DECL. */
@@ -963,9 +966,11 @@ privatize_symbol_name_1 (symtab_node *node, tree decl)
return false;
name = maybe_rewrite_identifier (name);
+ unsigned &clone_number = lto_clone_numbers->get_or_insert (name);
symtab->change_decl_assembler_name (decl,
- clone_function_name_numbered (
- name, "lto_priv"));
+ clone_function_name (
+ name, "lto_priv", clone_number));
+ clone_number++;
if (node->lto_file_data)
lto_record_renamed_decl (node->lto_file_data, name,
@@ -1157,6 +1162,8 @@ lto_promote_cross_file_statics (void)
part->encoder = compute_ltrans_boundary (part->encoder);
}
+ lto_clone_numbers = new hash_map<const char *, unsigned>;
+
/* Look at boundaries and promote symbols as needed. */
for (i = 0; i < n_sets; i++)
{
@@ -1187,6 +1194,7 @@ lto_promote_cross_file_statics (void)
promote_symbol (node);
}
}
+ delete lto_clone_numbers;
}
/* Rename statics in the whole unit in the case that
@@ -1196,9 +1204,12 @@ void
lto_promote_statics_nonwpa (void)
{
symtab_node *node;
+
+ lto_clone_numbers = new hash_map<const char *, unsigned>;
FOR_EACH_SYMBOL (node)
{
rename_statics (NULL, node);
validize_symbol_for_target (node);
}
+ delete lto_clone_numbers;
}
--
2.19.1
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2018-11-30 21:11 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-16 19:38 [PATCH] " Michael Ploujnikov
2018-07-17 6:10 ` Bernhard Reutner-Fischer
2018-07-17 10:03 ` Richard Biener
2018-07-17 20:25 ` Michael Ploujnikov
2018-07-20 2:49 ` Michael Ploujnikov
2018-07-20 10:05 ` Richard Biener
2018-07-24 13:57 ` Michael Ploujnikov
2018-07-26 17:27 ` Michael Ploujnikov
2018-07-31 17:40 ` Michael Ploujnikov
2018-08-01 10:38 ` Richard Biener
2018-08-02 19:05 ` Michael Ploujnikov
2018-08-13 23:58 ` [PING][PATCH] " Michael Ploujnikov
2018-08-20 19:47 ` Jeff Law
2018-08-31 18:49 ` [PING v2][PATCH] " Michael Ploujnikov
2018-09-03 10:02 ` Martin Jambor
2018-09-03 11:08 ` Richard Biener
2018-09-03 13:15 ` Martin Jambor
2018-09-05 3:24 ` Michael Ploujnikov
2018-11-28 21:09 ` [PATCH v3] " Michael Ploujnikov
2018-11-28 22:49 ` Segher Boessenkool
2018-11-29 14:13 ` Michael Ploujnikov
2018-11-30 7:26 ` Richard Biener
2018-11-30 21:11 ` Michael Ploujnikov [this message]
2018-12-01 16:31 ` H.J. Lu
2018-12-03 17:00 ` Michael Ploujnikov
2018-12-04 3:06 ` Michael Ploujnikov
2018-12-04 14:07 ` Jan Hubicka
2018-09-05 3:32 ` [PING v2][PATCH] " Michael Ploujnikov
2018-12-04 12:48 ` Martin Jambor
2018-12-04 13:22 ` Michael Ploujnikov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fa12f39f-6d95-c3ab-39f9-06481239e234@oracle.com \
--to=michael.ploujnikov@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=mjambor@suse.cz \
--cc=richard.guenther@gmail.com \
--cc=segher@kernel.crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).