[RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support

[RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support

[Venkataramanan Kumar]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

Hi Maintainers,

This patch adds macros to support gprof in Aarch64. The difference
from the previous patch is that the compiler, while generating
"mcount" routine for an instrumented function, also passes the return
address as argument.

The "mcount" routine in glibc will be modified as follows.

(-----Snip-----)
 #define MCOUNT \
-void __mcount (void)                                                         \
+void __mcount (void* frompc)
               \
 {                                                                            \
-  mcount_internal ((u_long) RETURN_ADDRESS (1), (u_long) RETURN_ADDRESS (0)); \
+  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
 }
(-----Snip-----)

Also in Aarch64 cases__builtin_return_adderess(n) where n>0, still be
returning 0 as it was doing before.

If this is Ok I will send the patch to glibc as well.

2013-08-02  Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>

         * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
           (NO_PROFILE_COUNTERS): Likewise.
           (PROFILE_HOOK): Likewise.
           (FUNCTION_PROFILER): Likewise.
        *  config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
           .

regards,
Venkat.

[-- Attachment #2: gcc.gprof.diff --]
[-- Type: application/octet-stream, Size: 2237 bytes --]

Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c	(revision 201441)
+++ gcc/config/aarch64/aarch64.c	(working copy)
@@ -3856,13 +3856,6 @@
   output_addr_const (f, x);
 }
 
-void
-aarch64_function_profiler (FILE *f ATTRIBUTE_UNUSED,
-			   int labelno ATTRIBUTE_UNUSED)
-{
-  sorry ("function profiling");
-}
-
 bool
 aarch64_label_mentioned_p (rtx x)
 {
Index: gcc/config/aarch64/aarch64.h
===================================================================
--- gcc/config/aarch64/aarch64.h	(revision 201441)
+++ gcc/config/aarch64/aarch64.h	(working copy)
@@ -781,9 +781,23 @@
 #define PRINT_OPERAND_ADDRESS(STREAM, X) \
   aarch64_print_operand_address (STREAM, X)
 
-#define FUNCTION_PROFILER(STREAM, LABELNO) \
-  aarch64_function_profiler (STREAM, LABELNO)
+#define MCOUNT_NAME "_mcount"
 
+#define NO_PROFILE_COUNTERS 1
+
+/* Emit rtl for profiling.  Output assembler code to FILE
+   to call "_mcount" for profiling a function entry.  */
+#define PROFILE_HOOK(LABEL)                                  \
+{                                                            \
+  rtx fun,lr;                                                \
+  lr = get_hard_reg_initial_val (Pmode, LR_REGNUM);          \
+  fun = gen_rtx_SYMBOL_REF (Pmode, MCOUNT_NAME);             \
+  emit_library_call (fun, LCT_NORMAL, VOIDmode, 1,lr,Pmode); \
+}
+
+/* All the work done in PROFILE_HOOK, but still required.  */
+#define FUNCTION_PROFILER(STREAM, LABELNO) do { } while (0)
+
 /* For some reason, the Linux headers think they know how to define
    these macros.  They don't!!!  */
 #undef ASM_APP_ON
Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 201441)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -487,13 +487,6 @@
 	return 0
     }
 
-    # We don't yet support profiling for AArch64.
-    if { [istarget aarch64*-*-*]
-	 && ([lindex $test_what 1] == "-p"
-	     || [lindex $test_what 1] == "-pg") } {
-	return 0
-    }
-
     # cygwin does not support -p.
     if { [istarget *-*-cygwin*] && $test_what == "-p" } {
 	return 0

Re: [RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support

[Venkataramanan Kumar]

ping!

On 3 August 2013 23:31, Venkataramanan Kumar
<venkataramanan.kumar@linaro.org> wrote:
> Hi Maintainers,
>
> This patch adds macros to support gprof in Aarch64. The difference
> from the previous patch is that the compiler, while generating
> "mcount" routine for an instrumented function, also passes the return
> address as argument.
>
> The "mcount" routine in glibc will be modified as follows.
>
> (-----Snip-----)
>  #define MCOUNT \
> -void __mcount (void)                                                         \
> +void __mcount (void* frompc)
>                \
>  {                                                                            \
> -  mcount_internal ((u_long) RETURN_ADDRESS (1), (u_long) RETURN_ADDRESS (0)); \
> +  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
>  }
> (-----Snip-----)
>
> Also in Aarch64 cases__builtin_return_adderess(n) where n>0, still be
> returning 0 as it was doing before.
>
> If this is Ok I will send the patch to glibc as well.
>
> 2013-08-02  Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>
>
>          * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
>            (NO_PROFILE_COUNTERS): Likewise.
>            (PROFILE_HOOK): Likewise.
>            (FUNCTION_PROFILER): Likewise.
>         *  config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
>            .
>
> regards,
> Venkat.

Re: [RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support

[Marcus Shawcroft]

On 03/08/13 19:01, Venkataramanan Kumar wrote:


> 2013-08-02  Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>
>
>           * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
>             (NO_PROFILE_COUNTERS): Likewise.
>             (PROFILE_HOOK): Likewise.
>             (FUNCTION_PROFILER): Likewise.
>          *  config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
>             .
>
> regards,
> Venkat.
>

Hi Venkat,

Looking at the various other ports it looks that the majority choose to 
use FUNCTION_PROFILER_HOOK rather than PROFILE_HOOK.

Using PROFILE_HOOK to inject a regular call to to _mcount() means that 
all arguments passed in registers in every function will be spilled and 
reloaded because the _mcount call will kill the caller save registers.

Using the FUNCTION_PROFILER_HOOK and taking care not to kill the caller 
save registers would be less invasive.  The LR argument to _mcount would 
need to be passed in a temporary register, say x9 and _mcount would also 
need to ensure caller save registers are saved and restored.

The latter seems to be a better option to me, is there compelling reason 
to choose PROFILE_HOOK over FUNCTION_PROFILER_HOOK ??

Cheers
/Marcus

Re: [RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support

[Matthew Gretton-Dann]

Marcus,

On 9 August 2013 18:17, Marcus Shawcroft <marcus.shawcroft@arm.com> wrote:
> On 03/08/13 19:01, Venkataramanan Kumar wrote:
>
>
>> 2013-08-02  Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>
>>
>>           * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
>>             (NO_PROFILE_COUNTERS): Likewise.
>>             (PROFILE_HOOK): Likewise.
>>             (FUNCTION_PROFILER): Likewise.
>>          *  config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
>>             .
>>
>> regards,
>> Venkat.
>>
>
> Hi Venkat,
>
> Looking at the various other ports it looks that the majority choose to use
> FUNCTION_PROFILER_HOOK rather than PROFILE_HOOK.
>
> Using PROFILE_HOOK to inject a regular call to to _mcount() means that all
> arguments passed in registers in every function will be spilled and reloaded
> because the _mcount call will kill the caller save registers.
>
> Using the FUNCTION_PROFILER_HOOK and taking care not to kill the caller save
> registers would be less invasive.  The LR argument to _mcount would need to
> be passed in a temporary register, say x9 and _mcount would also need to
> ensure caller save registers are saved and restored.
>
> The latter seems to be a better option to me, is there compelling reason to
> choose PROFILE_HOOK over FUNCTION_PROFILER_HOOK ??

(I think you mean FUNCTION_PROFILER rather than FUNCTION_PROFILER_HOOK
in all the above.)

Using either PROFILE_HOOK or FUNCTION_PROFILER results in a call chain
that looks like the following (assuming the C Library is glibc):

 Function -> _mcount -> _mcount_internal.

Where _mcount_internal is the C function that does the real work and
is provided in glibc.  Importantly this means that _mcount_internal
follows the normal ABI - so we have to save the caller saved registers
somewhere.

Using FUNCTION_PROFILER requires us to write assembler which saves and
restores all caller saved registers every time it is called, and
requires (as you say) a special ABI.  This means _mcount ends up being
a piece of assembly that saves all caller-saved registers (i.e.
parameter-passing & temporary registers) and then makes the call to
_mcount internal before restoring everything on _mcount's return.

Using PROFILE_HOOK will cause the compiler to do all the heavy
lifting, and it will do the minimum required (for example with a
function with one parameter it will only save and restore x0).
_mcount in this case can be a simple function that sets up some
parameters and calls _mcount_internal (or even _mcount could just
alias _mcount_internal).

As to which of PROFILE_HOOK or FUNCTION_PROFILER are "the right way"
(TM) - I don't know - the documentation isn't very clear at all.
PROFILE_HOOK was introduced to support profiling for AIX 4.3.
http://gcc.gnu.org/ml/gcc-patches/2000-12/msg00580.html is the initial
patch, with a reworked patch here:
http://gcc.gnu.org/ml/gcc-patches/2001-02/msg00112.html. The final
commit happening on 2001-02-05.  The patch was introduced because it
was impossible to make FUNCTION_PROFILER work for AIX 4.3 and so a new
hook that worked earlier in the compiler was needed.  There doesn't
seem to have been a discussion about preferring one form over the
other.

In conclusion - I prefer the PROFILE_HOOK method because it makes the
compiler do all the work, and results in less impact on stack usage
and performance.  FUNCTION_PROFILER may impact the code generated by
the compiler less and produce a smaller overall image - but I'm not
sure that's more beneficial.

Thanks,

Matt


-- 
Matthew Gretton-Dann
Linaro Toolchain Working Group
matthew.gretton-dann@linaro.org

Re: [RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support

[Marcus Shawcroft]

Hi Venkat,

On 3 August 2013 19:01, Venkataramanan Kumar
<venkataramanan.kumar@linaro.org> wrote:

> This patch adds macros to support gprof in Aarch64. The difference
> from the previous patch is that the compiler, while generating
> "mcount" routine for an instrumented function, also passes the return
> address as argument.
>
> The "mcount" routine in glibc will be modified as follows.
>
> (-----Snip-----)
>  #define MCOUNT \
> -void __mcount (void)                                                         \
> +void __mcount (void* frompc)
>                \
>  {                                                                            \
> -  mcount_internal ((u_long) RETURN_ADDRESS (1), (u_long) RETURN_ADDRESS (0)); \
> +  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
>  }
> (-----Snip-----)


> If this is Ok I will send the patch to glibc as well.

> 2013-08-02  Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>
>
>          * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
>            (NO_PROFILE_COUNTERS): Likewise.
>            (PROFILE_HOOK): Likewise.
>            (FUNCTION_PROFILER): Likewise.
>         *  config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
>            .
>
> regards,
> Venkat.

+  emit_library_call (fun, LCT_NORMAL, VOIDmode, 1,lr,Pmode); \
+}

GNU coding style requires spaces after the commas, but otherwise I
have no further comments on this patch. Post the glibc patch please.

Thanks
/Marcus

Re: [RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support

[Venkataramanan Kumar]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

Hi Marcus,

I have re-based the patch and tested for aarch64-none-elf with no regressions.
Also for aarch64-unknown-linux-gnu the following test cases passes.

Before:
--------
UNSUPPORTED: gcc.dg/nested-func-4.c
UNSUPPORTED: gcc.dg/pr43643.c:
UNSUPPORTED: gcc.dg/nest.c
UNSUPPORTED: gcc.dg/20021014-1.c
UNSUPPORTED: gcc.dg/pr32450.c
UNSUPPORTED: g++.dg/other/profile1.C -std=gnu++98
UNSUPPORTED: g++.dg/other/profile1.C -std=gnu++11

After:
-------
PASS: gcc.dg/nested-func-4.c (test for excess errors)
PASS: gcc.dg/nested-func-4.c execution test
PASS: gcc.dg/pr43643.c (test for excess errors)
PASS: gcc.dg/pr43643.c execution test
PASS: gcc.dg/nest.c (test for excess errors)
PASS: gcc.dg/nest.c execution test
PASS: gcc.dg/20021014-1.c (test for excess errors)
PASS: gcc.dg/20021014-1.c execution test
PASS: gcc.dg/pr32450.c (test for excess errors)
PASS: gcc.dg/pr32450.c execution test
PASS: g++.dg/other/profile1.C -std=gnu++98 (test for excess errors)
PASS: g++.dg/other/profile1.C -std=gnu++98 execution test
PASS: g++.dg/other/profile1.C -std=gnu++11 (test for excess errors)
PASS: g++.dg/other/profile1.C -std=gnu++11 execution test

Please let me know if I can commit it to trunk, given that glibc
patches are upstreamed.

2013-10-28  Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>

       * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
       (NO_PROFILE_COUNTERS): Likewise.
       (PROFILE_HOOK): Likewise.
       (FUNCTION_PROFILER): Likewise.
       *  config/aarch64/aarch64.c (aarch64_function_profiler): Remove.

regards,
Venkat.

On 27 August 2013 13:05, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote:
> Hi Venkat,
>
> On 3 August 2013 19:01, Venkataramanan Kumar
> <venkataramanan.kumar@linaro.org> wrote:
>
>> This patch adds macros to support gprof in Aarch64. The difference
>> from the previous patch is that the compiler, while generating
>> "mcount" routine for an instrumented function, also passes the return
>> address as argument.
>>
>> The "mcount" routine in glibc will be modified as follows.
>>
>> (-----Snip-----)
>>  #define MCOUNT \
>> -void __mcount (void)                                                         \
>> +void __mcount (void* frompc)
>>                \
>>  {                                                                            \
>> -  mcount_internal ((u_long) RETURN_ADDRESS (1), (u_long) RETURN_ADDRESS (0)); \
>> +  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
>>  }
>> (-----Snip-----)
>
>
>> If this is Ok I will send the patch to glibc as well.
>
>> 2013-08-02  Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>
>>
>>          * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
>>            (NO_PROFILE_COUNTERS): Likewise.
>>            (PROFILE_HOOK): Likewise.
>>            (FUNCTION_PROFILER): Likewise.
>>         *  config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
>>            .
>>
>> regards,
>> Venkat.
>
> +  emit_library_call (fun, LCT_NORMAL, VOIDmode, 1,lr,Pmode); \
> +}
>
> GNU coding style requires spaces after the commas, but otherwise I
> have no further comments on this patch. Post the glibc patch please.
>
> Thanks
> /Marcus

[-- Attachment #2: gcc.gprof.txt --]
[-- Type: text/plain, Size: 2249 bytes --]

Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c	(revision 202934)
+++ gcc/config/aarch64/aarch64.c	(working copy)
@@ -3857,13 +3857,6 @@
   output_addr_const (f, x);
 }
 
-void
-aarch64_function_profiler (FILE *f ATTRIBUTE_UNUSED,
-			   int labelno ATTRIBUTE_UNUSED)
-{
-  sorry ("function profiling");
-}
-
 bool
 aarch64_label_mentioned_p (rtx x)
 {
Index: gcc/config/aarch64/aarch64.h
===================================================================
--- gcc/config/aarch64/aarch64.h	(revision 202934)
+++ gcc/config/aarch64/aarch64.h	(working copy)
@@ -783,9 +783,23 @@
 #define PRINT_OPERAND_ADDRESS(STREAM, X) \
   aarch64_print_operand_address (STREAM, X)
 
-#define FUNCTION_PROFILER(STREAM, LABELNO) \
-  aarch64_function_profiler (STREAM, LABELNO)
+#define MCOUNT_NAME "_mcount"
 
+#define NO_PROFILE_COUNTERS 1
+
+/* Emit rtl for profiling.  Output assembler code to FILE
+   to call "_mcount" for profiling a function entry.  */
+#define PROFILE_HOOK(LABEL)                                    \
+{                                                              \
+  rtx fun,lr;                                                  \
+  lr = get_hard_reg_initial_val (Pmode, LR_REGNUM);            \
+  fun = gen_rtx_SYMBOL_REF (Pmode, MCOUNT_NAME);               \
+  emit_library_call (fun, LCT_NORMAL, VOIDmode, 1, lr, Pmode); \
+}
+
+/* All the work done in PROFILE_HOOK, but still required.  */
+#define FUNCTION_PROFILER(STREAM, LABELNO) do { } while (0)
+
 /* For some reason, the Linux headers think they know how to define
    these macros.  They don't!!!  */
 #undef ASM_APP_ON
Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 202934)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -494,13 +494,6 @@
 	return 0
     }
 
-    # We don't yet support profiling for AArch64.
-    if { [istarget aarch64*-*-*]
-	 && ([lindex $test_what 1] == "-p"
-	     || [lindex $test_what 1] == "-pg") } {
-	return 0
-    }
-
     # cygwin does not support -p.
     if { [istarget *-*-cygwin*] && $test_what == "-p" } {
 	return 0

Re: [RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support

[Marcus Shawcroft]

On 28 September 2013 11:57, Venkataramanan Kumar
<venkataramanan.kumar@linaro.org> wrote:

> 2013-10-28  Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>
>
>        * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
>        (NO_PROFILE_COUNTERS): Likewise.
>        (PROFILE_HOOK): Likewise.
>        (FUNCTION_PROFILER): Likewise.
>        *  config/aarch64/aarch64.c (aarch64_function_profiler): Remove.

OK, Thank you.
/Marcus