[PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[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);

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Jeff Law]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

[-- 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);

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Jeff Law]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

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
>

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Jeff Law]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[David Edelsohn]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Jeff Law]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Gerald Pfeifer]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[David Edelsohn]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

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

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Uros Bizjak]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

[-- 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);

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Uros Bizjak]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Uros Bizjak]

[-- 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)

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

[-- 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)

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Jack Howarth]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Uros Bizjak]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

[-- 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)

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Uros Bizjak]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Richard Henderson]

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~

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[H.J. Lu]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Uros Bizjak]

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.

Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[H.J. Lu]

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.