diff mbox

[09/10] ppc: Move exception generation code out of line

Message ID 1465795496-15071-10-git-send-email-clg@kaod.org (mailing list archive)
State New, archived
Headers show

Commit Message

Cédric Le Goater June 13, 2016, 5:24 a.m. UTC 
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

There's no point inlining this, if you hit the exception case you exit
anyway, and not inlining saves about 100K of code size (and cache
footprint).

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 target-ppc/translate.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Thomas Huth June 13, 2016, 7:44 a.m. UTC | #1 
On 13.06.2016 07:24, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> There's no point inlining this, if you hit the exception case you exit
> anyway, and not inlining saves about 100K of code size (and cache
> footprint).
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  target-ppc/translate.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index f211d175c09c..600d5db2bb9a 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -283,7 +283,8 @@ void gen_update_current_nip(void *opaque)
>      tcg_gen_movi_tl(cpu_nip, ctx->nip);
>  }
>  
> -static inline void gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t error)
> +static void __attribute__((noinline))
> +gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t error)
>  {
>      TCGv_i32 t0, t1;
>      if (ctx->exception == POWERPC_EXCP_NONE) {
> @@ -297,7 +298,8 @@ static inline void gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t
>      ctx->exception = (excp);
>  }
>  
> -static inline void gen_exception(DisasContext *ctx, uint32_t excp)
> +static void __attribute__((noinline))
> +gen_exception(DisasContext *ctx, uint32_t excp)
>  {
>      TCGv_i32 t0;
>      if (ctx->exception == POWERPC_EXCP_NONE) {
> @@ -309,7 +311,8 @@ static inline void gen_exception(DisasContext *ctx, uint32_t excp)
>      ctx->exception = (excp);
>  }
>  
> -static inline void gen_debug_exception(DisasContext *ctx)
> +static void __attribute__((noinline))
> +gen_debug_exception(DisasContext *ctx)
>  {
>      TCGv_i32 t0;

Do you get the same results if you just remove the "inline" keyword,
without adding the "__attribute__((noinline))" ? If yes, I'd suggest to
do this patch without the "__attribute__((noinline))" - that's easier to
read, and the compiler can still decide to inline something in case it's
better one a certain architecture.

 Thomas
Cédric Le Goater June 13, 2016, 8:36 a.m. UTC | #2 
On 06/13/2016 09:44 AM, Thomas Huth wrote:
> On 13.06.2016 07:24, Cédric Le Goater wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> There's no point inlining this, if you hit the exception case you exit
>> anyway, and not inlining saves about 100K of code size (and cache
>> footprint).
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> ---
>>  target-ppc/translate.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> index f211d175c09c..600d5db2bb9a 100644
>> --- a/target-ppc/translate.c
>> +++ b/target-ppc/translate.c
>> @@ -283,7 +283,8 @@ void gen_update_current_nip(void *opaque)
>>      tcg_gen_movi_tl(cpu_nip, ctx->nip);
>>  }
>>  
>> -static inline void gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t error)
>> +static void __attribute__((noinline))
>> +gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t error)
>>  {
>>      TCGv_i32 t0, t1;
>>      if (ctx->exception == POWERPC_EXCP_NONE) {
>> @@ -297,7 +298,8 @@ static inline void gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t
>>      ctx->exception = (excp);
>>  }
>>  
>> -static inline void gen_exception(DisasContext *ctx, uint32_t excp)
>> +static void __attribute__((noinline))
>> +gen_exception(DisasContext *ctx, uint32_t excp)
>>  {
>>      TCGv_i32 t0;
>>      if (ctx->exception == POWERPC_EXCP_NONE) {
>> @@ -309,7 +311,8 @@ static inline void gen_exception(DisasContext *ctx, uint32_t excp)
>>      ctx->exception = (excp);
>>  }
>>  
>> -static inline void gen_debug_exception(DisasContext *ctx)
>> +static void __attribute__((noinline))
>> +gen_debug_exception(DisasContext *ctx)
>>  {
>>      TCGv_i32 t0;
> 
> Do you get the same results if you just remove the "inline" keyword,
> without adding the "__attribute__((noinline))" ? If yes, I'd suggest to
> do this patch without the "__attribute__((noinline))" - that's easier to
> read, and the compiler can still decide to inline something in case it's
> better one a certain architecture.

Yes. They are no differences. 

The interesting part though is that the .text is about the same size. 
There is even a slight increase of ~2K with gcc 4.9.2 (intel host) and 
a slight decrease of ~1K with gcc 5.3.1 (ppc64le host).

I guess we can just drop that patch. It does not seem to bring much.

Thanks,

C.
David Gibson June 15, 2016, 1:57 a.m. UTC | #3 
On Mon, Jun 13, 2016 at 10:36:24AM +0200, Cédric Le Goater wrote:
> On 06/13/2016 09:44 AM, Thomas Huth wrote:
> > On 13.06.2016 07:24, Cédric Le Goater wrote:
> >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>
> >> There's no point inlining this, if you hit the exception case you exit
> >> anyway, and not inlining saves about 100K of code size (and cache
> >> footprint).
> >>
> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> ---
> >>  target-ppc/translate.c | 9 ++++++---
> >>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> >> index f211d175c09c..600d5db2bb9a 100644
> >> --- a/target-ppc/translate.c
> >> +++ b/target-ppc/translate.c
> >> @@ -283,7 +283,8 @@ void gen_update_current_nip(void *opaque)
> >>      tcg_gen_movi_tl(cpu_nip, ctx->nip);
> >>  }
> >>  
> >> -static inline void gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t error)
> >> +static void __attribute__((noinline))
> >> +gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t error)
> >>  {
> >>      TCGv_i32 t0, t1;
> >>      if (ctx->exception == POWERPC_EXCP_NONE) {
> >> @@ -297,7 +298,8 @@ static inline void gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t
> >>      ctx->exception = (excp);
> >>  }
> >>  
> >> -static inline void gen_exception(DisasContext *ctx, uint32_t excp)
> >> +static void __attribute__((noinline))
> >> +gen_exception(DisasContext *ctx, uint32_t excp)
> >>  {
> >>      TCGv_i32 t0;
> >>      if (ctx->exception == POWERPC_EXCP_NONE) {
> >> @@ -309,7 +311,8 @@ static inline void gen_exception(DisasContext *ctx, uint32_t excp)
> >>      ctx->exception = (excp);
> >>  }
> >>  
> >> -static inline void gen_debug_exception(DisasContext *ctx)
> >> +static void __attribute__((noinline))
> >> +gen_debug_exception(DisasContext *ctx)
> >>  {
> >>      TCGv_i32 t0;
> > 
> > Do you get the same results if you just remove the "inline" keyword,
> > without adding the "__attribute__((noinline))" ? If yes, I'd suggest to
> > do this patch without the "__attribute__((noinline))" - that's easier to
> > read, and the compiler can still decide to inline something in case it's
> > better one a certain architecture.
> 
> Yes. They are no differences. 
> 
> The interesting part though is that the .text is about the same size. 
> There is even a slight increase of ~2K with gcc 4.9.2 (intel host) and 
> a slight decrease of ~1K with gcc 5.3.1 (ppc64le host).
> 
> I guess we can just drop that patch. It does not seem to bring much.

I would prefer to see the inline keyword removed.  Except in the case
of tiny header functions, it's very rarely a good idea - usually the
compiler will have better information on whether to inline or not.
diff mbox

Patch

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index f211d175c09c..600d5db2bb9a 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -283,7 +283,8 @@  void gen_update_current_nip(void *opaque)
     tcg_gen_movi_tl(cpu_nip, ctx->nip);
 }
 
-static inline void gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t error)
+static void __attribute__((noinline))
+gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t error)
 {
     TCGv_i32 t0, t1;
     if (ctx->exception == POWERPC_EXCP_NONE) {
@@ -297,7 +298,8 @@  static inline void gen_exception_err(DisasContext *ctx, uint32_t excp, uint32_t
     ctx->exception = (excp);
 }
 
-static inline void gen_exception(DisasContext *ctx, uint32_t excp)
+static void __attribute__((noinline))
+gen_exception(DisasContext *ctx, uint32_t excp)
 {
     TCGv_i32 t0;
     if (ctx->exception == POWERPC_EXCP_NONE) {
@@ -309,7 +311,8 @@  static inline void gen_exception(DisasContext *ctx, uint32_t excp)
     ctx->exception = (excp);
 }
 
-static inline void gen_debug_exception(DisasContext *ctx)
+static void __attribute__((noinline))
+gen_debug_exception(DisasContext *ctx)
 {
     TCGv_i32 t0;