* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
@ 2015-04-29 16:48 Uros Bizjak
2015-04-29 17:57 ` Caroline Tice
0 siblings, 1 reply; 27+ messages in thread
From: Uros Bizjak @ 2015-04-29 16:48 UTC (permalink / raw)
To: gcc-patches; +Cc: cmtice, Jeff Law, Richard Henderson
Hello!
> 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.
This patch caused PR 65929 [1].
Targets are (ab)using ASM_DECLARE_FUNCTION_NAME and
ASM_DECLARE_FUNCTION_SIZE for more things that their name suggests. As
shown in the PR, some directives that are generated through these
macros apply only to real functions, not to parts of function in the
middle of the function.
In the particular case in the PR, assembler doesn't tolerate nested
function declaration.
I suggest to revert the patch for now, until side effects of the patch
are resolved.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65929
Uros.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-29 16:48 [PATCH] Fix size & type for cold partition names (hot-cold function partitioning) Uros Bizjak
@ 2015-04-29 17:57 ` Caroline Tice
2015-04-29 18:12 ` Uros Bizjak
0 siblings, 1 reply; 27+ messages in thread
From: Caroline Tice @ 2015-04-29 17:57 UTC (permalink / raw)
To: Uros Bizjak; +Cc: gcc-patches, Jeff Law, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]
The attached patch can revert the previous patch, if that is the way
we should proceed on this. If you want me to apply the reversion,
please let me know.
I would be happy to fix to the problem, rather than just reverting the
patch, but I do not have expertise in assembly language on other
platforms, so I would need some help, if anyone would be interested in
helping me?
-- Caroline Tice
cmtice@google.com
ChangeLog (gcc):
2015-04-29 Caroline Tice <cmtice@google.com>
Revert patch from 2015-04-27
* final.c (final_scan_insn): Remove code to output
cold_function_name as a function type in assembly.
* varasm.c (cold_function_name): Remove global declaration.
(assemble_start_function): Remove code to re-set
cold_function_name.
(assemble_end_function): Do not output side of cold
partition.
ChangLog (gcc/testsuite):
2015-04-29 Caroline Tice <cmtice@google.com>
Revert patch from 2015-04-27
* gcc.dg/tree-prof/cold_partition_label.c: Do not check
for cold partition size.
On Wed, Apr 29, 2015 at 9:33 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
>> 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.
>
> This patch caused PR 65929 [1].
>
> Targets are (ab)using ASM_DECLARE_FUNCTION_NAME and
> ASM_DECLARE_FUNCTION_SIZE for more things that their name suggests. As
> shown in the PR, some directives that are generated through these
> macros apply only to real functions, not to parts of function in the
> middle of the function.
>
> In the particular case in the PR, assembler doesn't tolerate nested
> function declaration.
>
> I suggest to revert the patch for now, until side effects of the patch
> are resolved.
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65929
>
> Uros.
[-- Attachment #2: revert-cold-partition-name.patch --]
[-- Type: text/x-patch, Size: 3340 bytes --]
Index: gcc/final.c
===================================================================
--- gcc/final.c (revision 222584)
+++ gcc/final.c (working copy)
@@ -2233,16 +2233,10 @@
suffixing "cold" to the original function's name. */
if (in_cold_section_p)
{
- cold_function_name
+ tree 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 222584)
+++ gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (working copy)
@@ -35,6 +35,4 @@
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 222584)
+++ gcc/varasm.c (working copy)
@@ -187,13 +187,6 @@
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;
@@ -1726,7 +1719,6 @@
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
{
@@ -1864,12 +1856,6 @@
save_text_section = in_section;
switch_to_section (unlikely_text_section ());
-#ifdef ASM_DECLARE_FUNCTION_SIZE
- if (cold_function_name != NULL_TREE)
- ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
- IDENTIFIER_POINTER (cold_function_name),
- decl);
-#endif
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 222584)
+++ gcc/varasm.h (working copy)
@@ -20,13 +20,6 @@
#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);
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-29 17:57 ` Caroline Tice
@ 2015-04-29 18:12 ` Uros Bizjak
2015-04-29 18:52 ` Uros Bizjak
0 siblings, 1 reply; 27+ messages in thread
From: Uros Bizjak @ 2015-04-29 18:12 UTC (permalink / raw)
To: Caroline Tice; +Cc: gcc-patches, Jeff Law, Richard Henderson
On Wed, Apr 29, 2015 at 7:38 PM, Caroline Tice <cmtice@google.com> wrote:
> The attached patch can revert the previous patch, if that is the way
> we should proceed on this. If you want me to apply the reversion,
> please let me know.
>
> I would be happy to fix to the problem, rather than just reverting the
> patch, but I do not have expertise in assembly language on other
> platforms, so I would need some help, if anyone would be interested in
> helping me?
How about adding ASM_DECLARE_COLD_FUNCTION_NAME and
ASM_DECLARE_COLD_FUNCTION_SIZE? If these are defined, they can be used
instead, and targets are free to define them in any way.
Uros.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-29 18:12 ` Uros Bizjak
@ 2015-04-29 18:52 ` Uros Bizjak
2015-04-29 19:08 ` Caroline Tice
0 siblings, 1 reply; 27+ messages in thread
From: Uros Bizjak @ 2015-04-29 18:52 UTC (permalink / raw)
To: Caroline Tice; +Cc: gcc-patches, Jeff Law, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]
On Wed, Apr 29, 2015 at 7:47 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Apr 29, 2015 at 7:38 PM, Caroline Tice <cmtice@google.com> wrote:
>> The attached patch can revert the previous patch, if that is the way
>> we should proceed on this. If you want me to apply the reversion,
>> please let me know.
>>
>> I would be happy to fix to the problem, rather than just reverting the
>> patch, but I do not have expertise in assembly language on other
>> platforms, so I would need some help, if anyone would be interested in
>> helping me?
>
> How about adding ASM_DECLARE_COLD_FUNCTION_NAME and
> ASM_DECLARE_COLD_FUNCTION_SIZE? If these are defined, they can be used
> instead, and targets are free to define them in any way.
Something like the attached prototype RFC patch. Using this patch,
readelf -sW returns:
Symbol table '.symtab' contains 18 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000000 0 SECTION LOCAL DEFAULT 1
2: 0000000000000000 0 SECTION LOCAL DEFAULT 3
3: 0000000000000000 0 SECTION LOCAL DEFAULT 4
4: 0000000000000000 0 SECTION LOCAL DEFAULT 5
5: 0000000000000000 0 SECTION LOCAL DEFAULT 6
6: 0000000000000000 0 SECTION LOCAL DEFAULT 8
7: 0000000000000000 8 FUNC LOCAL DEFAULT 6 main.cold.0
8: 0000000000000000 0 SECTION LOCAL DEFAULT 10
9: 0000000000000000 0 SECTION LOCAL DEFAULT 13
10: 0000000000000000 0 SECTION LOCAL DEFAULT 12
11: 0000000000000000 312 FUNC GLOBAL DEFAULT [<other>: 88] 8 main
12: 0000000000000008 160 OBJECT GLOBAL DEFAULT COM buf
13: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND memset
14: 0000000000000000 44 FUNC GLOBAL DEFAULT [<other>: 88] 1 sub2
15: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND strcmp
16: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND exit
17: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND abort
Uros.
[-- Attachment #2: a.diff.txt --]
[-- Type: text/plain, Size: 2752 bytes --]
Index: config/elfos.h
===================================================================
--- config/elfos.h (revision 222585)
+++ config/elfos.h (working copy)
@@ -284,6 +284,17 @@ see the files COPYING3 and COPYING.RUNTIME respect
while (0)
#endif
+#ifndef ASM_DECLARE_COLD_FUNCTION_NAME
+#define ASM_DECLARE_COLD_FUNCTION_NAME(FILE, NAME, DECL) \
+ do \
+ { \
+ ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function"); \
+ ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL)); \
+ ASM_OUTPUT_FUNCTION_LABEL (FILE, NAME, DECL); \
+ } \
+ while (0)
+#endif
+
/* Write the extra assembler code needed to declare an object properly. */
#ifdef HAVE_GAS_GNU_UNIQUE_OBJECT
@@ -358,6 +369,16 @@ see the files COPYING3 and COPYING.RUNTIME respect
while (0)
#endif
+#ifndef ASM_DECLARE_COLD_FUNCTION_SIZE
+#define ASM_DECLARE_COLD_FUNCTION_SIZE(FILE, FNAME, DECL) \
+ do \
+ { \
+ if (!flag_inhibit_size_directive) \
+ ASM_OUTPUT_MEASURED_SIZE (FILE, FNAME); \
+ } \
+ while (0)
+#endif
+
/* A table of bytes codes used by the ASM_OUTPUT_ASCII and
ASM_OUTPUT_LIMITED_STRING macros. Each byte in the table
corresponds to a particular byte value [0..255]. For any
Index: final.c
===================================================================
--- final.c (revision 222585)
+++ final.c (working copy)
@@ -2235,10 +2235,10 @@ final_scan_insn (rtx_insn *insn, FILE *file, int o
{
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);
+#ifdef ASM_DECLARE_COLD_FUNCTION_NAME
+ ASM_DECLARE_COLD_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));
Index: varasm.c
===================================================================
--- varasm.c (revision 222585)
+++ varasm.c (working copy)
@@ -1864,11 +1864,10 @@ assemble_end_function (tree decl, const char *fnna
save_text_section = in_section;
switch_to_section (unlikely_text_section ());
-#ifdef ASM_DECLARE_FUNCTION_SIZE
+#ifdef ASM_DECLARE_COLD_FUNCTION_SIZE
if (cold_function_name != NULL_TREE)
- ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
- IDENTIFIER_POINTER (cold_function_name),
- decl);
+ ASM_DECLARE_COLD_FUNCTION_SIZE
+ (asm_out_file, IDENTIFIER_POINTER (cold_function_name), decl);
#endif
ASM_OUTPUT_LABEL (asm_out_file, crtl->subsections.cold_section_end_label);
if (first_function_block_is_cold)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-29 18:52 ` Uros Bizjak
@ 2015-04-29 19:08 ` Caroline Tice
2015-04-29 21:30 ` Caroline Tice
0 siblings, 1 reply; 27+ messages in thread
From: Caroline Tice @ 2015-04-29 19:08 UTC (permalink / raw)
To: Uros Bizjak; +Cc: gcc-patches, Jeff Law, Richard Henderson
Thank you; I will work with your suggestions and try to get a new
patch done soon.
-- Caroline Tice
cmtice@google.com
On Wed, Apr 29, 2015 at 11:34 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Apr 29, 2015 at 7:47 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Wed, Apr 29, 2015 at 7:38 PM, Caroline Tice <cmtice@google.com> wrote:
>>> The attached patch can revert the previous patch, if that is the way
>>> we should proceed on this. If you want me to apply the reversion,
>>> please let me know.
>>>
>>> I would be happy to fix to the problem, rather than just reverting the
>>> patch, but I do not have expertise in assembly language on other
>>> platforms, so I would need some help, if anyone would be interested in
>>> helping me?
>>
>> How about adding ASM_DECLARE_COLD_FUNCTION_NAME and
>> ASM_DECLARE_COLD_FUNCTION_SIZE? If these are defined, they can be used
>> instead, and targets are free to define them in any way.
>
> Something like the attached prototype RFC patch. Using this patch,
> readelf -sW returns:
>
> Symbol table '.symtab' contains 18 entries:
> Num: Value Size Type Bind Vis Ndx Name
> 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
> 1: 0000000000000000 0 SECTION LOCAL DEFAULT 1
> 2: 0000000000000000 0 SECTION LOCAL DEFAULT 3
> 3: 0000000000000000 0 SECTION LOCAL DEFAULT 4
> 4: 0000000000000000 0 SECTION LOCAL DEFAULT 5
> 5: 0000000000000000 0 SECTION LOCAL DEFAULT 6
> 6: 0000000000000000 0 SECTION LOCAL DEFAULT 8
> 7: 0000000000000000 8 FUNC LOCAL DEFAULT 6 main.cold.0
> 8: 0000000000000000 0 SECTION LOCAL DEFAULT 10
> 9: 0000000000000000 0 SECTION LOCAL DEFAULT 13
> 10: 0000000000000000 0 SECTION LOCAL DEFAULT 12
> 11: 0000000000000000 312 FUNC GLOBAL DEFAULT [<other>: 88] 8 main
> 12: 0000000000000008 160 OBJECT GLOBAL DEFAULT COM buf
> 13: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND memset
> 14: 0000000000000000 44 FUNC GLOBAL DEFAULT [<other>: 88] 1 sub2
> 15: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND strcmp
> 16: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND exit
> 17: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND abort
>
> Uros.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
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
0 siblings, 2 replies; 27+ messages in thread
From: Caroline Tice @ 2015-04-29 21:30 UTC (permalink / raw)
To: Uros Bizjak; +Cc: gcc-patches, Jeff Law, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 3825 bytes --]
Here is a new patch to update the cold name partition so that it will
only be treated like a function name and be given a size on the
architectures that specifically define macros for such.
I also updated the test case to try to only test on the appropriate
architectures. I am not sure I got the target triples correct for
this, so I would appreciate some extra attention to that in the
review. I have tested this new patch on my workstation and it works
as intended. I am in the process of bootstrapping with the new patch.
Assuming that the bootstrap passes, is this ok to commit?
-- Caroline Tice
cmtice@google.com
ChangeLog (gcc):
2015-04-29 Caroline Tice <cmtice@google.com>
PR 65929
* config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
(ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
* final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
* varasm.c (assemble_end_function): Use ASM_DECLARE_COLD_FUNCTION_SIZE
instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
ChangeLog (testsuite):
2015-04-29 Caroline Tice <cmtice@google.com>
PR 65929
* gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
partition size on certain targets.
On Wed, Apr 29, 2015 at 11:59 AM, Caroline Tice <cmtice@google.com> wrote:
> Thank you; I will work with your suggestions and try to get a new
> patch done soon.
>
> -- Caroline Tice
> cmtice@google.com
>
>
> On Wed, Apr 29, 2015 at 11:34 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Wed, Apr 29, 2015 at 7:47 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> On Wed, Apr 29, 2015 at 7:38 PM, Caroline Tice <cmtice@google.com> wrote:
>>>> The attached patch can revert the previous patch, if that is the way
>>>> we should proceed on this. If you want me to apply the reversion,
>>>> please let me know.
>>>>
>>>> I would be happy to fix to the problem, rather than just reverting the
>>>> patch, but I do not have expertise in assembly language on other
>>>> platforms, so I would need some help, if anyone would be interested in
>>>> helping me?
>>>
>>> How about adding ASM_DECLARE_COLD_FUNCTION_NAME and
>>> ASM_DECLARE_COLD_FUNCTION_SIZE? If these are defined, they can be used
>>> instead, and targets are free to define them in any way.
>>
>> Something like the attached prototype RFC patch. Using this patch,
>> readelf -sW returns:
>>
>> Symbol table '.symtab' contains 18 entries:
>> Num: Value Size Type Bind Vis Ndx Name
>> 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
>> 1: 0000000000000000 0 SECTION LOCAL DEFAULT 1
>> 2: 0000000000000000 0 SECTION LOCAL DEFAULT 3
>> 3: 0000000000000000 0 SECTION LOCAL DEFAULT 4
>> 4: 0000000000000000 0 SECTION LOCAL DEFAULT 5
>> 5: 0000000000000000 0 SECTION LOCAL DEFAULT 6
>> 6: 0000000000000000 0 SECTION LOCAL DEFAULT 8
>> 7: 0000000000000000 8 FUNC LOCAL DEFAULT 6 main.cold.0
>> 8: 0000000000000000 0 SECTION LOCAL DEFAULT 10
>> 9: 0000000000000000 0 SECTION LOCAL DEFAULT 13
>> 10: 0000000000000000 0 SECTION LOCAL DEFAULT 12
>> 11: 0000000000000000 312 FUNC GLOBAL DEFAULT [<other>: 88] 8 main
>> 12: 0000000000000008 160 OBJECT GLOBAL DEFAULT COM buf
>> 13: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND memset
>> 14: 0000000000000000 44 FUNC GLOBAL DEFAULT [<other>: 88] 1 sub2
>> 15: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND strcmp
>> 16: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND exit
>> 17: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND abort
>>
>> Uros.
[-- Attachment #2: new-cold-partition-name.patch --]
[-- Type: text/x-patch, Size: 3676 bytes --]
Index: gcc/config/elfos.h
===================================================================
--- gcc/config/elfos.h (revision 222584)
+++ gcc/config/elfos.h (working copy)
@@ -284,6 +284,22 @@
while (0)
#endif
+/* Write the extra assembler code needed to declare the name of a
+ cold function partition properly. Some svr4 assemblers need to also
+ have something extra said about the function's return value. We
+ allow for that here. */
+
+#ifndef ASM_DECLARE_COLD_FUNCTION_NAME
+#define ASM_DECLARE_COLD_FUNCTION_NAME(FILE, NAME, DECL) \
+ do \
+ { \
+ ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function"); \
+ ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL)); \
+ ASM_OUTPUT_FUNCTION_LABEL (FILE, NAME, DECL); \
+ } \
+ while (0)
+#endif
+
/* Write the extra assembler code needed to declare an object properly. */
#ifdef HAVE_GAS_GNU_UNIQUE_OBJECT
@@ -358,6 +374,17 @@
while (0)
#endif
+/* This is how to declare the size of a cold function partition. */
+#ifndef ASM_DECLARE_COLD_FUNCTION_SIZE
+#define ASM_DECLARE_COLD_FUNCTION_SIZE(FILE, FNAME, DECL) \
+ do \
+ { \
+ if (!flag_inhibit_size_directive) \
+ ASM_OUTPUT_MEASURED_SIZE (FILE, FNAME); \
+ } \
+ while (0)
+#endif
+
/* A table of bytes codes used by the ASM_OUTPUT_ASCII and
ASM_OUTPUT_LIMITED_STRING macros. Each byte in the table
corresponds to a particular byte value [0..255]. For any
Index: gcc/final.c
===================================================================
--- gcc/final.c (revision 222587)
+++ gcc/final.c (working copy)
@@ -2235,10 +2235,11 @@
{
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);
+#ifdef ASM_DECLARE_COLD_FUNCTION_NAME
+ ASM_DECLARE_COLD_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));
Index: gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (revision 222588)
+++ gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (working copy)
@@ -35,6 +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 { 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 { cleanup-saved-temps } } */
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c (revision 222587)
+++ gcc/varasm.c (working copy)
@@ -1864,11 +1864,11 @@
save_text_section = in_section;
switch_to_section (unlikely_text_section ());
-#ifdef ASM_DECLARE_FUNCTION_SIZE
+#ifdef ASM_DECLARE_COLD_FUNCTION_SIZE
if (cold_function_name != NULL_TREE)
- ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
- IDENTIFIER_POINTER (cold_function_name),
- decl);
+ ASM_DECLARE_COLD_FUNCTION_SIZE (asm_out_file,
+ IDENTIFIER_POINTER (cold_function_name),
+ decl);
#endif
ASM_OUTPUT_LABEL (asm_out_file, crtl->subsections.cold_section_end_label);
if (first_function_block_is_cold)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-29 21:30 ` Caroline Tice
@ 2015-04-30 4:15 ` Jack Howarth
2015-04-30 6:40 ` Uros Bizjak
1 sibling, 0 replies; 27+ messages in thread
From: Jack Howarth @ 2015-04-30 4:15 UTC (permalink / raw)
To: Caroline Tice; +Cc: Uros Bizjak, gcc-patches, Jeff Law, Richard Henderson
The new patch bootstraps fine on x86_64-apple-darwin14.
On Wed, Apr 29, 2015 at 5:22 PM, Caroline Tice <cmtice@google.com> wrote:
> Here is a new patch to update the cold name partition so that it will
> only be treated like a function name and be given a size on the
> architectures that specifically define macros for such.
>
> I also updated the test case to try to only test on the appropriate
> architectures. I am not sure I got the target triples correct for
> this, so I would appreciate some extra attention to that in the
> review. I have tested this new patch on my workstation and it works
> as intended. I am in the process of bootstrapping with the new patch.
> Assuming that the bootstrap passes, is this ok to commit?
>
> -- Caroline Tice
> cmtice@google.com
>
> ChangeLog (gcc):
>
> 2015-04-29 Caroline Tice <cmtice@google.com>
>
> PR 65929
> * config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
> (ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
> * final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
> instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
> * varasm.c (assemble_end_function): Use ASM_DECLARE_COLD_FUNCTION_SIZE
> instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
>
> ChangeLog (testsuite):
>
> 2015-04-29 Caroline Tice <cmtice@google.com>
>
> PR 65929
> * gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
> partition size on certain targets.
>
>
>
>
>
> On Wed, Apr 29, 2015 at 11:59 AM, Caroline Tice <cmtice@google.com> wrote:
>> Thank you; I will work with your suggestions and try to get a new
>> patch done soon.
>>
>> -- Caroline Tice
>> cmtice@google.com
>>
>>
>> On Wed, Apr 29, 2015 at 11:34 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> On Wed, Apr 29, 2015 at 7:47 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>>> On Wed, Apr 29, 2015 at 7:38 PM, Caroline Tice <cmtice@google.com> wrote:
>>>>> The attached patch can revert the previous patch, if that is the way
>>>>> we should proceed on this. If you want me to apply the reversion,
>>>>> please let me know.
>>>>>
>>>>> I would be happy to fix to the problem, rather than just reverting the
>>>>> patch, but I do not have expertise in assembly language on other
>>>>> platforms, so I would need some help, if anyone would be interested in
>>>>> helping me?
>>>>
>>>> How about adding ASM_DECLARE_COLD_FUNCTION_NAME and
>>>> ASM_DECLARE_COLD_FUNCTION_SIZE? If these are defined, they can be used
>>>> instead, and targets are free to define them in any way.
>>>
>>> Something like the attached prototype RFC patch. Using this patch,
>>> readelf -sW returns:
>>>
>>> Symbol table '.symtab' contains 18 entries:
>>> Num: Value Size Type Bind Vis Ndx Name
>>> 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
>>> 1: 0000000000000000 0 SECTION LOCAL DEFAULT 1
>>> 2: 0000000000000000 0 SECTION LOCAL DEFAULT 3
>>> 3: 0000000000000000 0 SECTION LOCAL DEFAULT 4
>>> 4: 0000000000000000 0 SECTION LOCAL DEFAULT 5
>>> 5: 0000000000000000 0 SECTION LOCAL DEFAULT 6
>>> 6: 0000000000000000 0 SECTION LOCAL DEFAULT 8
>>> 7: 0000000000000000 8 FUNC LOCAL DEFAULT 6 main.cold.0
>>> 8: 0000000000000000 0 SECTION LOCAL DEFAULT 10
>>> 9: 0000000000000000 0 SECTION LOCAL DEFAULT 13
>>> 10: 0000000000000000 0 SECTION LOCAL DEFAULT 12
>>> 11: 0000000000000000 312 FUNC GLOBAL DEFAULT [<other>: 88] 8 main
>>> 12: 0000000000000008 160 OBJECT GLOBAL DEFAULT COM buf
>>> 13: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND memset
>>> 14: 0000000000000000 44 FUNC GLOBAL DEFAULT [<other>: 88] 1 sub2
>>> 15: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND strcmp
>>> 16: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND exit
>>> 17: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND abort
>>>
>>> Uros.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
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
1 sibling, 1 reply; 27+ messages in thread
From: Uros Bizjak @ 2015-04-30 6:40 UTC (permalink / raw)
To: Caroline Tice; +Cc: gcc-patches, Jeff Law, Richard Henderson
On Wed, Apr 29, 2015 at 11:22 PM, Caroline Tice <cmtice@google.com> wrote:
> Here is a new patch to update the cold name partition so that it will
> only be treated like a function name and be given a size on the
> architectures that specifically define macros for such.
>
> I also updated the test case to try to only test on the appropriate
> architectures. I am not sure I got the target triples correct for
> this, so I would appreciate some extra attention to that in the
> review. I have tested this new patch on my workstation and it works
> as intended. I am in the process of bootstrapping with the new patch.
> Assuming that the bootstrap passes, is this ok to commit?
>
> -- Caroline Tice
> cmtice@google.com
>
> ChangeLog (gcc):
>
> 2015-04-29 Caroline Tice <cmtice@google.com>
>
> PR 65929
> * config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
> (ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
> * final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
> instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
> * varasm.c (assemble_end_function): Use ASM_DECLARE_COLD_FUNCTION_SIZE
> instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
>
> ChangeLog (testsuite):
>
> 2015-04-29 Caroline Tice <cmtice@google.com>
>
> PR 65929
> * gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
> partition size on certain targets.
Documentation for new macros is missing (please see doc/tm.texi.in).
Uros.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
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
0 siblings, 2 replies; 27+ messages in thread
From: Caroline Tice @ 2015-04-30 16:36 UTC (permalink / raw)
To: Uros Bizjak; +Cc: gcc-patches, Jeff Law, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 2636 bytes --]
Done. Here is the updated patch (with ChangeLog entries). Only
change was to update tm.texi.in.
The bootstrap passed. Is the patch ok to commit?
-- Caroline
cmtice@google.com
ChangeLog (gcc):
2015-04-30 Caroline Tice <cmtice@google.com>
PR 65929
* config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
(ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
* doc/tm.texi.in (ASM_DECLARE_COLD_FUNCTION_NAME): Document new macro.
(ASM_DECLARE_COLD_FUNCTION_SIZE): Document new macro.
* final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
* varasm.c (assemble_end_function): Use ASM_DECLARE_COLD_FUNCTION_SIZE
instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
ChangeLog (gcc/testsuite):
2015-04-30 Caroline Tice <cmtice@google.com>
PR 65929
* gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
partition size on certain targets.
On Wed, Apr 29, 2015 at 11:12 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Apr 29, 2015 at 11:22 PM, Caroline Tice <cmtice@google.com> wrote:
>> Here is a new patch to update the cold name partition so that it will
>> only be treated like a function name and be given a size on the
>> architectures that specifically define macros for such.
>>
>> I also updated the test case to try to only test on the appropriate
>> architectures. I am not sure I got the target triples correct for
>> this, so I would appreciate some extra attention to that in the
>> review. I have tested this new patch on my workstation and it works
>> as intended. I am in the process of bootstrapping with the new patch.
>> Assuming that the bootstrap passes, is this ok to commit?
>>
>> -- Caroline Tice
>> cmtice@google.com
>>
>> ChangeLog (gcc):
>>
>> 2015-04-29 Caroline Tice <cmtice@google.com>
>>
>> PR 65929
>> * config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
>> (ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
>> * final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
>> instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
>> * varasm.c (assemble_end_function): Use ASM_DECLARE_COLD_FUNCTION_SIZE
>> instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
>>
>> ChangeLog (testsuite):
>>
>> 2015-04-29 Caroline Tice <cmtice@google.com>
>>
>> PR 65929
>> * gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
>> partition size on certain targets.
>
> Documentation for new macros is missing (please see doc/tm.texi.in).
>
> Uros.
[-- Attachment #2: new-cold-partition-name.patch --]
[-- Type: text/x-patch, Size: 5423 bytes --]
Index: gcc/config/elfos.h
===================================================================
--- gcc/config/elfos.h (revision 222635)
+++ gcc/config/elfos.h (working copy)
@@ -284,6 +284,22 @@
while (0)
#endif
+/* Write the extra assembler code needed to declare the name of a
+ cold function partition properly. Some svr4 assemblers need to also
+ have something extra said about the function's return value. We
+ allow for that here. */
+
+#ifndef ASM_DECLARE_COLD_FUNCTION_NAME
+#define ASM_DECLARE_COLD_FUNCTION_NAME(FILE, NAME, DECL) \
+ do \
+ { \
+ ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function"); \
+ ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL)); \
+ ASM_OUTPUT_FUNCTION_LABEL (FILE, NAME, DECL); \
+ } \
+ while (0)
+#endif
+
/* Write the extra assembler code needed to declare an object properly. */
#ifdef HAVE_GAS_GNU_UNIQUE_OBJECT
@@ -358,6 +374,17 @@
while (0)
#endif
+/* This is how to declare the size of a cold function partition. */
+#ifndef ASM_DECLARE_COLD_FUNCTION_SIZE
+#define ASM_DECLARE_COLD_FUNCTION_SIZE(FILE, FNAME, DECL) \
+ do \
+ { \
+ if (!flag_inhibit_size_directive) \
+ ASM_OUTPUT_MEASURED_SIZE (FILE, FNAME); \
+ } \
+ while (0)
+#endif
+
/* A table of bytes codes used by the ASM_OUTPUT_ASCII and
ASM_OUTPUT_LIMITED_STRING macros. Each byte in the table
corresponds to a particular byte value [0..255]. For any
Index: gcc/doc/tm.texi.in
===================================================================
--- gcc/doc/tm.texi.in (revision 222635)
+++ gcc/doc/tm.texi.in (working copy)
@@ -5574,6 +5574,34 @@
of this macro.
@end defmac
+@defmac ASM_DECLARE_COLD_FUNCTION_NAME (@var{stream}, @var{name}, @var{decl})
+A C statement (sans semicolon) to output to the stdio stream
+@var{stream} any text necessary for declaring the name @var{name} of a
+cold function partition which is being defined. This macro is responsible
+for outputting the label definition (perhaps using
+@code{ASM_OUTPUT_FUNCTION_LABEL}). The argument @var{decl} is the
+@code{FUNCTION_DECL} tree node representing the function.
+
+If this macro is not defined, then the cold partition name is defined in the
+usual manner as a label (by means of @code{ASM_OUTPUT_LABEL}).
+
+You may wish to use @code{ASM_OUTPUT_TYPE_DIRECTIVE} in the definition
+of this macro.
+@end defmac
+
+@defmac ASM_DECLARE_COLD_FUNCTION_SIZE (@var{stream}, @var{name}, @var{decl})
+A C statement (sans semicolon) to output to the stdio stream
+@var{stream} any text necessary for declaring the size of a cold function
+partition which is being defined. The argument @var{name} is the name of the
+cold partition of the function. The argument @var{decl} is the
+@code{FUNCTION_DECL} tree node representing the function.
+
+If this macro is not defined, then the partition size is not defined.
+
+You may wish to use @code{ASM_OUTPUT_MEASURED_SIZE} in the definition
+of this macro.
+@end defmac
+
@defmac ASM_DECLARE_OBJECT_NAME (@var{stream}, @var{name}, @var{decl})
A C statement (sans semicolon) to output to the stdio stream
@var{stream} any text necessary for declaring the name @var{name} of an
Index: gcc/final.c
===================================================================
--- gcc/final.c (revision 222635)
+++ gcc/final.c (working copy)
@@ -2235,10 +2235,11 @@
{
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);
+#ifdef ASM_DECLARE_COLD_FUNCTION_NAME
+ ASM_DECLARE_COLD_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));
Index: gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (revision 222635)
+++ gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (working copy)
@@ -35,6 +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 { 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 { cleanup-saved-temps } } */
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c (revision 222635)
+++ gcc/varasm.c (working copy)
@@ -1864,11 +1864,11 @@
save_text_section = in_section;
switch_to_section (unlikely_text_section ());
-#ifdef ASM_DECLARE_FUNCTION_SIZE
+#ifdef ASM_DECLARE_COLD_FUNCTION_SIZE
if (cold_function_name != NULL_TREE)
- ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
- IDENTIFIER_POINTER (cold_function_name),
- decl);
+ ASM_DECLARE_COLD_FUNCTION_SIZE (asm_out_file,
+ IDENTIFIER_POINTER (cold_function_name),
+ decl);
#endif
ASM_OUTPUT_LABEL (asm_out_file, crtl->subsections.cold_section_end_label);
if (first_function_block_is_cold)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-30 16:36 ` Caroline Tice
@ 2015-04-30 17:32 ` Uros Bizjak
2015-04-30 17:44 ` Richard Henderson
1 sibling, 0 replies; 27+ messages in thread
From: Uros Bizjak @ 2015-04-30 17:32 UTC (permalink / raw)
To: Caroline Tice; +Cc: gcc-patches, Jeff Law, Richard Henderson
On Thu, Apr 30, 2015 at 6:26 PM, Caroline Tice <cmtice@google.com> wrote:
> Done. Here is the updated patch (with ChangeLog entries). Only
> change was to update tm.texi.in.
>
> The bootstrap passed. Is the patch ok to commit?
FYI, the (previous) patch also looks good on alphaev68-linux-gnu
native bootstrap [1].
[1] https://gcc.gnu.org/ml/gcc-testresults/2015-04/msg03508.html
Uros.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
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
1 sibling, 1 reply; 27+ messages in thread
From: Richard Henderson @ 2015-04-30 17:44 UTC (permalink / raw)
To: Caroline Tice, Uros Bizjak; +Cc: gcc-patches, Jeff Law
On 04/30/2015 09:26 AM, Caroline Tice wrote:
> 2015-04-30 Caroline Tice <cmtice@google.com>
>
> PR 65929
> * config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
> (ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
> * doc/tm.texi.in (ASM_DECLARE_COLD_FUNCTION_NAME): Document new macro.
> (ASM_DECLARE_COLD_FUNCTION_SIZE): Document new macro.
> * final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
> instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
> * varasm.c (assemble_end_function): Use ASM_DECLARE_COLD_FUNCTION_SIZE
> instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
>
> ChangeLog (gcc/testsuite):
>
> 2015-04-30 Caroline Tice <cmtice@google.com>
>
> PR 65929
> * gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
> partition size on certain targets.
Ok.
r~
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-30 17:44 ` Richard Henderson
@ 2015-04-30 19:34 ` H.J. Lu
2015-04-30 19:34 ` Caroline Tice
0 siblings, 1 reply; 27+ messages in thread
From: H.J. Lu @ 2015-04-30 19:34 UTC (permalink / raw)
To: Richard Henderson; +Cc: Caroline Tice, Uros Bizjak, gcc-patches, Jeff Law
On Thu, Apr 30, 2015 at 10:38 AM, Richard Henderson <rth@redhat.com> wrote:
> On 04/30/2015 09:26 AM, Caroline Tice wrote:
>> 2015-04-30 Caroline Tice <cmtice@google.com>
>>
>> PR 65929
>> * config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
>> (ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
>> * doc/tm.texi.in (ASM_DECLARE_COLD_FUNCTION_NAME): Document new macro.
This breaks bootstrap. You also need to regenerate and
check in doc/tm.texi.
--
H.J.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
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
0 siblings, 2 replies; 27+ messages in thread
From: Caroline Tice @ 2015-04-30 19:34 UTC (permalink / raw)
To: H.J. Lu; +Cc: Richard Henderson, Uros Bizjak, gcc-patches, Jeff Law
What is the correct way to regenerate doc/tm.texi?
-- Caroline
On Thu, Apr 30, 2015 at 12:24 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Apr 30, 2015 at 10:38 AM, Richard Henderson <rth@redhat.com> wrote:
>> On 04/30/2015 09:26 AM, Caroline Tice wrote:
>>> 2015-04-30 Caroline Tice <cmtice@google.com>
>>>
>>> PR 65929
>>> * config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
>>> (ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
>>> * doc/tm.texi.in (ASM_DECLARE_COLD_FUNCTION_NAME): Document new macro.
>
> This breaks bootstrap. You also need to regenerate and
> check in doc/tm.texi.
>
> --
> H.J.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-30 19:34 ` Caroline Tice
@ 2015-04-30 19:38 ` Uros Bizjak
2015-04-30 19:42 ` H.J. Lu
1 sibling, 0 replies; 27+ messages in thread
From: Uros Bizjak @ 2015-04-30 19:38 UTC (permalink / raw)
To: Caroline Tice; +Cc: H.J. Lu, Richard Henderson, gcc-patches, Jeff Law
On Thu, Apr 30, 2015 at 9:33 PM, Caroline Tice <cmtice@google.com> wrote:
> What is the correct way to regenerate doc/tm.texi?
I have just committed the fix.
Uros.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-30 19:34 ` Caroline Tice
2015-04-30 19:38 ` Uros Bizjak
@ 2015-04-30 19:42 ` H.J. Lu
1 sibling, 0 replies; 27+ messages in thread
From: H.J. Lu @ 2015-04-30 19:42 UTC (permalink / raw)
To: Caroline Tice; +Cc: Richard Henderson, Uros Bizjak, gcc-patches, Jeff Law
On Thu, Apr 30, 2015 at 12:33 PM, Caroline Tice <cmtice@google.com> wrote:
> What is the correct way to regenerate doc/tm.texi?
>
Verify that you have permission to grant a GFDL license for all
new text in tm.texi, then copy it to ../../src-trunk/gcc/doc/tm.texi.
--
H.J.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-28 17:33 ` David Edelsohn
@ 2015-04-28 17:36 ` Caroline Tice
0 siblings, 0 replies; 27+ messages in thread
From: Caroline Tice @ 2015-04-28 17:36 UTC (permalink / raw)
To: David Edelsohn; +Cc: Jeffrey Law, GCC Patches
Thank you!
-- Caroline Tice
cmtice@google.com
On Tue, Apr 28, 2015 at 10:16 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
> I just committed the patch for Dominique.
>
> - David
>
>
> On Tue, Apr 28, 2015 at 1:12 PM, Caroline Tice <cmtice@google.com> wrote:
>> Yes, this is already mentioned in PR 65910
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65910).
>>
>> There is a patch mentioned in that PR that both appears to fix the
>> problem and appears to have been approved. I thought the author of
>> the patch would commit it, but that does not appear to have happened
>> yet. Should I go ahead and commit that patch, or should I wait for
>> the author to do that?
>>
>> -- Caroline Tice
>> cmtice@google.clom
>>
>>
>> On Tue, Apr 28, 2015 at 10:10 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
>>> Caroline,
>>>
>>> Your patch has broken bootstrap on AIX and probably other platforms.
>>>
>>> /nasfarm/edelsohn/src/src/gcc/varasm.c: In function 'void
>>> assemble_end_function(tree, const char*)':
>>> /nasfarm/edelsohn/src/src/gcc/varasm.c:1870:12: error:
>>> 'ASM_DECLARE_FUNCTION_SIZE' was not declared in this scope
>>> decl);
>>> ^
>>>
>>> You added a reference to ASM_DECLARE_FUNCTION_SIZE without guarding
>>> it, as was done only 10 lines higher in the same function. The macro
>>> is not declared for all targets.
>>>
>>> Also, the ChangeLog entry has numerous typos.
>>>
>>> Please fix ASAP.
>>>
>>> Thanks, David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
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
2 siblings, 1 reply; 27+ messages in thread
From: David Edelsohn @ 2015-04-28 17:33 UTC (permalink / raw)
To: Caroline Tice; +Cc: Jeffrey Law, GCC Patches
I just committed the patch for Dominique.
- David
On Tue, Apr 28, 2015 at 1:12 PM, Caroline Tice <cmtice@google.com> wrote:
> Yes, this is already mentioned in PR 65910
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65910).
>
> There is a patch mentioned in that PR that both appears to fix the
> problem and appears to have been approved. I thought the author of
> the patch would commit it, but that does not appear to have happened
> yet. Should I go ahead and commit that patch, or should I wait for
> the author to do that?
>
> -- Caroline Tice
> cmtice@google.clom
>
>
> On Tue, Apr 28, 2015 at 10:10 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
>> Caroline,
>>
>> Your patch has broken bootstrap on AIX and probably other platforms.
>>
>> /nasfarm/edelsohn/src/src/gcc/varasm.c: In function 'void
>> assemble_end_function(tree, const char*)':
>> /nasfarm/edelsohn/src/src/gcc/varasm.c:1870:12: error:
>> 'ASM_DECLARE_FUNCTION_SIZE' was not declared in this scope
>> decl);
>> ^
>>
>> You added a reference to ASM_DECLARE_FUNCTION_SIZE without guarding
>> it, as was done only 10 lines higher in the same function. The macro
>> is not declared for all targets.
>>
>> Also, the ChangeLog entry has numerous typos.
>>
>> Please fix ASAP.
>>
>> Thanks, David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
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
2 siblings, 0 replies; 27+ messages in thread
From: Gerald Pfeifer @ 2015-04-28 17:17 UTC (permalink / raw)
To: Caroline Tice; +Cc: David Edelsohn, Jeffrey Law, gcc-patches
On Tue, 28 Apr 2015, Caroline Tice wrote:
> Yes, this is already mentioned in PR 65910
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65910).
>
> There is a patch mentioned in that PR that both appears to fix the
> problem and appears to have been approved. I thought the author of
> the patch would commit it, but that does not appear to have happened
> yet. Should I go ahead and commit that patch, or should I wait for
> the author to do that?
Unbreaking the tree has high priority, so if there is an approved
patch, go ahead and commit it.
Gerald
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
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
2 siblings, 0 replies; 27+ messages in thread
From: Jeff Law @ 2015-04-28 17:16 UTC (permalink / raw)
To: Caroline Tice, David Edelsohn; +Cc: GCC Patches
On 04/28/2015 11:12 AM, Caroline Tice wrote:
> Yes, this is already mentioned in PR 65910
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65910).
>
> There is a patch mentioned in that PR that both appears to fix the
> problem and appears to have been approved. I thought the author of
> the patch would commit it, but that does not appear to have happened
> yet. Should I go ahead and commit that patch, or should I wait for
> the author to do that?
If it's not committed, go ahead and do so.
jeff
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-04-28 17:14 David Edelsohn
@ 2015-04-28 17:15 ` Caroline Tice
2015-04-28 17:16 ` Jeff Law
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Caroline Tice @ 2015-04-28 17:15 UTC (permalink / raw)
To: David Edelsohn; +Cc: Jeffrey Law, GCC Patches
Yes, this is already mentioned in PR 65910
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65910).
There is a patch mentioned in that PR that both appears to fix the
problem and appears to have been approved. I thought the author of
the patch would commit it, but that does not appear to have happened
yet. Should I go ahead and commit that patch, or should I wait for
the author to do that?
-- Caroline Tice
cmtice@google.clom
On Tue, Apr 28, 2015 at 10:10 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
> Caroline,
>
> Your patch has broken bootstrap on AIX and probably other platforms.
>
> /nasfarm/edelsohn/src/src/gcc/varasm.c: In function 'void
> assemble_end_function(tree, const char*)':
> /nasfarm/edelsohn/src/src/gcc/varasm.c:1870:12: error:
> 'ASM_DECLARE_FUNCTION_SIZE' was not declared in this scope
> decl);
> ^
>
> You added a reference to ASM_DECLARE_FUNCTION_SIZE without guarding
> it, as was done only 10 lines higher in the same function. The macro
> is not declared for all targets.
>
> Also, the ChangeLog entry has numerous typos.
>
> Please fix ASAP.
>
> Thanks, David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
@ 2015-04-28 17:14 David Edelsohn
2015-04-28 17:15 ` Caroline Tice
0 siblings, 1 reply; 27+ messages in thread
From: David Edelsohn @ 2015-04-28 17:14 UTC (permalink / raw)
To: Caroline Tice; +Cc: Jeffrey Law, GCC Patches
Caroline,
Your patch has broken bootstrap on AIX and probably other platforms.
/nasfarm/edelsohn/src/src/gcc/varasm.c: In function 'void
assemble_end_function(tree, const char*)':
/nasfarm/edelsohn/src/src/gcc/varasm.c:1870:12: error:
'ASM_DECLARE_FUNCTION_SIZE' was not declared in this scope
decl);
^
You added a reference to ASM_DECLARE_FUNCTION_SIZE without guarding
it, as was done only 10 lines higher in the same function. The macro
is not declared for all targets.
Also, the ChangeLog entry has numerous typos.
Please fix ASAP.
Thanks, David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-03-31 15:13 ` Caroline Tice
@ 2015-03-31 15:17 ` Jeff Law
0 siblings, 0 replies; 27+ messages in thread
From: Jeff Law @ 2015-03-31 15:17 UTC (permalink / raw)
To: Caroline Tice; +Cc: GCC Patches
On 03/31/2015 09:12 AM, Caroline Tice wrote:
> I am fine with waiting until stage 1. When that is likely to be?
We're very close (week or two) to getting the first GCC 5 RCs spun, so
stage1 for GCC 6 should open shortly thereafter.
jeff
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-03-31 5:19 ` Jeff Law
@ 2015-03-31 15:13 ` Caroline Tice
2015-03-31 15:17 ` Jeff Law
0 siblings, 1 reply; 27+ messages in thread
From: Caroline Tice @ 2015-03-31 15:13 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
I am fine with waiting until stage 1. When that is likely to be?
-- Caroline Tice
cmtice@google.com
On Mon, Mar 30, 2015 at 10:19 PM, Jeff Law <law@redhat.com> wrote:
> On 03/27/2015 10:44 AM, Caroline Tice wrote:
>>
>> 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.
>
> Given we're late in stage4, can this wait until stage1, where it should be
> considered pre-approved? I'd hate to mess up the PA or PTX ports at this
> stage in the release process.
>
> jeff
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2015-03-27 16:45 ` Caroline Tice
@ 2015-03-31 5:19 ` Jeff Law
2015-03-31 15:13 ` Caroline Tice
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Law @ 2015-03-31 5:19 UTC (permalink / raw)
To: Caroline Tice; +Cc: GCC Patches
On 03/27/2015 10:44 AM, Caroline Tice wrote:
> 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.
Given we're late in stage4, can this wait until stage1, where it should
be considered pre-approved? I'd hate to mess up the PA or PTX ports at
this stage in the release process.
jeff
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2014-12-08 21:07 ` Jeff Law
@ 2015-03-27 16:45 ` Caroline Tice
2015-03-31 5:19 ` Jeff Law
0 siblings, 1 reply; 27+ messages in thread
From: Caroline Tice @ 2015-03-27 16:45 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
[-- 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);
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
2014-12-05 22:41 Caroline Tice
@ 2014-12-08 21:07 ` Jeff Law
2015-03-27 16:45 ` Caroline Tice
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Law @ 2014-12-08 21:07 UTC (permalink / raw)
To: Caroline Tice, GCC Patches
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)
@ 2014-12-05 22:41 Caroline Tice
2014-12-08 21:07 ` Jeff Law
0 siblings, 1 reply; 27+ messages in thread
From: Caroline Tice @ 2014-12-05 22:41 UTC (permalink / raw)
To: GCC Patches; +Cc: Caroline Tice
[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]
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.
[-- Attachment #2: cold-partition-name.patch --]
[-- Type: text/x-patch, Size: 2830 bytes --]
Index: gcc/final.c
===================================================================
--- gcc/final.c (revision 218434)
+++ gcc/final.c (working copy)
@@ -2223,10 +2223,16 @@
suffixing "cold" to the original function's name. */
if (in_cold_section_p)
{
- tree cold_function_name
+ cold_partition_name
= clone_function_name (current_function_decl, "cold");
+#ifdef ASM_DECLARE_FUNCTION_NAME
+ ASM_DECLARE_FUNCTION_NAME (asm_out_file,
+ IDENTIFIER_POINTER (cold_partition_name),
+ current_function_decl);
+#else
ASM_OUTPUT_LABEL (asm_out_file,
- IDENTIFIER_POINTER (cold_function_name));
+ IDENTIFIER_POINTER (cold_partition_name));
+#endif
}
break;
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c (revision 218434)
+++ gcc/varasm.c (working copy)
@@ -171,6 +171,13 @@
at the cold section. */
bool in_cold_section_p;
+/* The following global holds the partition 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_partition_name = NULL_TREE;
+
/* A linked list of all the unnamed sections. */
static GTY(()) section *unnamed_sections;
@@ -1708,6 +1715,7 @@
ASM_GENERATE_INTERNAL_LABEL (tmp_label, "LCOLDE", const_labelno);
crtl->subsections.cold_section_end_label = ggc_strdup (tmp_label);
const_labelno++;
+ cold_partition_name = NULL_TREE;
}
else
{
@@ -1843,6 +1851,10 @@
save_text_section = in_section;
switch_to_section (unlikely_text_section ());
+ if (cold_partition_name != NULL_TREE)
+ ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
+ IDENTIFIER_POINTER (cold_partition_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 218434)
+++ gcc/varasm.h (working copy)
@@ -20,6 +20,13 @@
#ifndef GCC_VARASM_H
#define GCC_VARASM_H
+/* The following global holds the partition 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_partition_name;
+
extern tree tree_output_constant_def (tree);
extern void make_decl_rtl (tree);
extern rtx make_decl_rtl_for_debug (tree);
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-04-30 19:34 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-29 16:48 [PATCH] Fix size & type for cold partition names (hot-cold function partitioning) 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
-- strict thread matches above, loose matches on Subject: below --
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
2014-12-05 22:41 Caroline Tice
2014-12-08 21:07 ` Jeff Law
2015-03-27 16:45 ` Caroline Tice
2015-03-31 5:19 ` Jeff Law
2015-03-31 15:13 ` Caroline Tice
2015-03-31 15:17 ` Jeff Law
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).