From: Caroline Tice <cmtice@google.com>
To: Jeff Law <law@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
Date: Fri, 27 Mar 2015 16:45:00 -0000 [thread overview]
Message-ID: <CABtf2+Ro9jO7Mfm0hrjCMV4x5UROhVTHzQY3jE7Fiw=zrb8faA@mail.gmail.com> (raw)
In-Reply-To: <548612F0.8050209@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3381 bytes --]
It took me a while to get a test case I'm happy with, so I'm
re-submitting the whole patch for approval.
2015-03-27 Caroline Tice <cmtice@google.com>
* final.c (final_scan_insn): Change 'cold_function_name' to
'cold_partition_name' and make it a global variable; also output
assembly to give it a 'FUNC' type, if appropriate.
* varasm.c (cold_partition_name): Declare and initialize global
variable.
(assemble_start_function): Re-set value for cold_partition_name.
(assemble_end_function): Output assembly to calculate size of cold
partition, and associate size with name, if appropriate.
* varash.h (cold_partition_name): Add extern declaration for global
variable.
* testsuite/gcc.dg/tree-prof/cold_partition_label.c: Add dg-final-use
to scan assembly for cold partition name and size.
I've attached the whole patch, with the testcase (which is the only
thing that has changed since the previous patch).
-- Caroline Tice
cmtice@google.com
On Mon, Dec 8, 2014 at 1:06 PM, Jeff Law <law@redhat.com> wrote:
> On 12/05/14 15:41, Caroline Tice wrote:
>>
>> When hot/cold function splitting occurs, a symbol is generated for the
>> cold partition, and gets output in the assembly & debug info, but the
>> symbol currently gets a size of 0 and a type of NOTYPE, as in this
>> example (on x86_64-linux) from the cold_partition_label test in the
>> testsuite:
>>
>> $ readelf -sW cold_partition_label.x02 | grep foo
>> 36: 0000000000400450 0 NOTYPE LOCAL DEFAULT 12 foo.cold.0
>> 58: 0000000000400490 43 FUNC GLOBAL DEFAULT 12 foo
>> $
>>
>> This patch fixes this by calculating the right size for the partition,
>> and outputing the size and type fo the cold partition symbol. After
>> applying this patch and looking at the same test, I get:
>>
>> $ readelf -sW cold_partition_label.x02 | grep foo
>> 36: 0000000000400450 29 FUNC LOCAL DEFAULT 12 foo.cold.0
>> 58: 0000000000400490 43 FUNC GLOBAL DEFAULT 12 foo
>> $
>>
>> This patch has been tested by bootstrapping the compiler, running the
>> dejagnu testsuite with no regressions, and checked as shown above that
>> it fixes the original problem. Is this patch OK to commit to ToT?
>>
>> -- Caroline Tice
>> cmtice@google.com
>>
>> 2014-12-05 Caroline Tice <cmtice@google.com>
>>
>> * final.c (final_scan_insn): Change 'cold_function_name' to
>> 'cold_partition_name' and make it a global variable; also output
>> assembly to give it a 'FUNC' type, if appropriate.
>> * varasm.c (cold_partition_name): Declare and initialize global
>> variable.
>> (assemble_start_function): Re-set value for cold_partition_name.
>> (assemble_end_function): Output assembly to calculate size of
>> cold
>> partition, and associate size with name, if appropriate.
>> * varash.h (cold_partition_name): Add extern declaration for
>> global
>> variable.
>
> OK for the trunk. I'm ever-so-slightly worried about the PA and PTX ports
> are they're probably the most sensitive to types. But we'll deal with them
> if they complain.
>
> Adding a testcase would be helpful. I believe some of the LTO tests use
> readelf, so there should be some infrastructure you can factor out and
> reuse.
>
> jeff
[-- Attachment #2: cold-partition-name.patch --]
[-- Type: text/x-patch, Size: 3298 bytes --]
Index: gcc/final.c
===================================================================
--- gcc/final.c (revision 221669)
+++ gcc/final.c (working copy)
@@ -2236,10 +2236,16 @@
suffixing "cold" to the original function's name. */
if (in_cold_section_p)
{
- tree cold_function_name
+ cold_function_name
= clone_function_name (current_function_decl, "cold");
+#ifdef ASM_DECLARE_FUNCTION_NAME
+ ASM_DECLARE_FUNCTION_NAME (asm_out_file,
+ IDENTIFIER_POINTER (cold_function_name),
+ current_function_decl);
+#else
ASM_OUTPUT_LABEL (asm_out_file,
IDENTIFIER_POINTER (cold_function_name));
+#endif
}
break;
Index: gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (revision 221669)
+++ gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (working copy)
@@ -35,4 +35,6 @@
return 0;
}
+/* { dg-final-use { scan-assembler "foo\[._\]+cold\[\._\]+0" } } */
+/* { dg-final-use { scan-assembler "size\[ \ta-zA-Z0-0\]+foo\[._\]+cold\[\._\]+0" } } */
/* { dg-final-use { cleanup-saved-temps } } */
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c (revision 221669)
+++ gcc/varasm.c (working copy)
@@ -187,6 +187,13 @@
at the cold section. */
bool in_cold_section_p;
+/* The following global holds the "function name" for the code in the
+ cold section of a function, if hot/cold function splitting is enabled
+ and there was actually code that went into the cold section. A
+ pseudo function name is needed for the cold section of code for some
+ debugging tools that perform symbolization. */
+tree cold_function_name = NULL_TREE;
+
/* A linked list of all the unnamed sections. */
static GTY(()) section *unnamed_sections;
@@ -1719,6 +1726,7 @@
ASM_GENERATE_INTERNAL_LABEL (tmp_label, "LCOLDE", const_labelno);
crtl->subsections.cold_section_end_label = ggc_strdup (tmp_label);
const_labelno++;
+ cold_function_name = NULL_TREE;
}
else
{
@@ -1856,6 +1864,10 @@
save_text_section = in_section;
switch_to_section (unlikely_text_section ());
+ if (cold_function_name != NULL_TREE)
+ ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
+ IDENTIFIER_POINTER (cold_function_name),
+ decl);
ASM_OUTPUT_LABEL (asm_out_file, crtl->subsections.cold_section_end_label);
if (first_function_block_is_cold)
switch_to_section (text_section);
Index: gcc/varasm.h
===================================================================
--- gcc/varasm.h (revision 221669)
+++ gcc/varasm.h (working copy)
@@ -20,6 +20,13 @@
#ifndef GCC_VARASM_H
#define GCC_VARASM_H
+/* The following global holds the "function name" for the code in the
+ cold section of a function, if hot/cold function splitting is enabled
+ and there was actually code that went into the cold section. A
+ pseudo function name is needed for the cold section of code for some
+ debugging tools that perform symbolization. */
+extern tree cold_function_name;
+
extern tree tree_output_constant_def (tree);
extern void make_decl_rtl (tree);
extern rtx make_decl_rtl_for_debug (tree);
next prev parent reply other threads:[~2015-03-27 16:45 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-05 22:41 Caroline Tice
2014-12-08 21:07 ` Jeff Law
2015-03-27 16:45 ` Caroline Tice [this message]
2015-03-31 5:19 ` Jeff Law
2015-03-31 15:13 ` Caroline Tice
2015-03-31 15:17 ` Jeff Law
2015-04-28 17:14 David Edelsohn
2015-04-28 17:15 ` Caroline Tice
2015-04-28 17:16 ` Jeff Law
2015-04-28 17:17 ` Gerald Pfeifer
2015-04-28 17:33 ` David Edelsohn
2015-04-28 17:36 ` Caroline Tice
2015-04-29 16:48 Uros Bizjak
2015-04-29 17:57 ` Caroline Tice
2015-04-29 18:12 ` Uros Bizjak
2015-04-29 18:52 ` Uros Bizjak
2015-04-29 19:08 ` Caroline Tice
2015-04-29 21:30 ` Caroline Tice
2015-04-30 4:15 ` Jack Howarth
2015-04-30 6:40 ` Uros Bizjak
2015-04-30 16:36 ` Caroline Tice
2015-04-30 17:32 ` Uros Bizjak
2015-04-30 17:44 ` Richard Henderson
2015-04-30 19:34 ` H.J. Lu
2015-04-30 19:34 ` Caroline Tice
2015-04-30 19:38 ` Uros Bizjak
2015-04-30 19:42 ` H.J. Lu
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='CABtf2+Ro9jO7Mfm0hrjCMV4x5UROhVTHzQY3jE7Fiw=zrb8faA@mail.gmail.com' \
--to=cmtice@google.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
/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).