All of lore.kernel.org
 help / color / mirror / Atom feed
From: mark.rutland@arm.com (Mark Rutland)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] compiler, clang: Add always_inline attribute to inline
Date: Tue, 20 Jun 2017 11:59:38 +0100	[thread overview]
Message-ID: <20170620105937.GD28157@leverpostej> (raw)
In-Reply-To: <f8ee882e9859f24dac24832bfb5ae0f1@codeaurora.org>

On Mon, Jun 19, 2017 at 03:19:27PM -0700, Sodagudi Prasad wrote:
> On 2017-06-19 14:42, David Rientjes wrote:
> >Yes, the arch/arm64/include/asm/cmpxchg.h instance appears to need
> >__always_inline as several other functions need __always_inline in
> >arch/arm64/include/*.  It's worth making that change as you
> >suggested in
> >your original patch.
> >
> >The concern, however, is inlining all "inline" functions
> >forcefully.  The
> >only reason this is done for gcc is because of suboptimal inlining
> >decisions in gcc < 4.
> >
> >So the question is whether this is a single instance that can be fixed
> >where clang un-inlining causes problems or whether that instance
> >suggests
> >all possible inline usage for clang absolutely requires __always_inline
> >due to a suboptimal compiler implementation.  I would suggest the
> >former.
> 
> Hi David,
> 
>  I am not 100% sure about the best approach for this problem. We may
> have to
> replace inline with always_inline for all inline functions where
> BUILD_BUG() used.
> 
> So far inline as always_inline for ARM64, if we do not continue same
> settings,
> will there not be any performance differences?
> 
> Hi Will and Mark,
> 
> Please suggest the best solution to this problem. Currently
> __xchg_mb is only having issue
> based on compiler -inline-threshold configuration. But there are
> many other instances
> in arch/arm64/* where BUILD_BUG() used for inline functions and
> which may fail later.

As with my reply to David, my preference would be that we:

1) Align compiler-clang.h with the compiler-gcc.h inlining behaviour, so
   that things work by default.

2) Fix up the arm64 core code (and drivers for architected / common
   peripherals) to use __always_inline where we always require inlining.

3) Have arm64 select CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING, and have
   people test-build configurations with CONFIG_OPTIMIZE_INLINING, with
   both GCC and clang.

4) Fix up drivers, etc, as appropriate.

5) Once that's largely stable, and if there's a benefit, have arm64
   select CONFIG_OPTIMIZE_INLINING by default.

That should avoid undue breakage, while enabling this ASAP.

Thanks,
Mark.

WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com>
To: Sodagudi Prasad <psodagud@codeaurora.org>
Cc: David Rientjes <rientjes@google.com>,
	will.deacon@arm.com, catalin.marinas@arm.com, mingo@kernel.org,
	peterz@infradead.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] compiler, clang: Add always_inline attribute to inline
Date: Tue, 20 Jun 2017 11:59:38 +0100	[thread overview]
Message-ID: <20170620105937.GD28157@leverpostej> (raw)
In-Reply-To: <f8ee882e9859f24dac24832bfb5ae0f1@codeaurora.org>

On Mon, Jun 19, 2017 at 03:19:27PM -0700, Sodagudi Prasad wrote:
> On 2017-06-19 14:42, David Rientjes wrote:
> >Yes, the arch/arm64/include/asm/cmpxchg.h instance appears to need
> >__always_inline as several other functions need __always_inline in
> >arch/arm64/include/*.  It's worth making that change as you
> >suggested in
> >your original patch.
> >
> >The concern, however, is inlining all "inline" functions
> >forcefully.  The
> >only reason this is done for gcc is because of suboptimal inlining
> >decisions in gcc < 4.
> >
> >So the question is whether this is a single instance that can be fixed
> >where clang un-inlining causes problems or whether that instance
> >suggests
> >all possible inline usage for clang absolutely requires __always_inline
> >due to a suboptimal compiler implementation.  I would suggest the
> >former.
> 
> Hi David,
> 
>  I am not 100% sure about the best approach for this problem. We may
> have to
> replace inline with always_inline for all inline functions where
> BUILD_BUG() used.
> 
> So far inline as always_inline for ARM64, if we do not continue same
> settings,
> will there not be any performance differences?
> 
> Hi Will and Mark,
> 
> Please suggest the best solution to this problem. Currently
> __xchg_mb is only having issue
> based on compiler -inline-threshold configuration. But there are
> many other instances
> in arch/arm64/* where BUILD_BUG() used for inline functions and
> which may fail later.

As with my reply to David, my preference would be that we:

1) Align compiler-clang.h with the compiler-gcc.h inlining behaviour, so
   that things work by default.

2) Fix up the arm64 core code (and drivers for architected / common
   peripherals) to use __always_inline where we always require inlining.

3) Have arm64 select CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING, and have
   people test-build configurations with CONFIG_OPTIMIZE_INLINING, with
   both GCC and clang.

4) Fix up drivers, etc, as appropriate.

5) Once that's largely stable, and if there's a benefit, have arm64
   select CONFIG_OPTIMIZE_INLINING by default.

That should avoid undue breakage, while enabling this ASAP.

Thanks,
Mark.

  reply	other threads:[~2017-06-20 10:59 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-13 22:39 Using __always_inline attribute Sodagudi Prasad
2017-06-13 22:39 ` Sodagudi Prasad
2017-06-14 10:06 ` Will Deacon
2017-06-14 10:06   ` Will Deacon
2017-06-14 22:33   ` Sodagudi Prasad
2017-06-14 22:33     ` Sodagudi Prasad
2017-06-15  0:50     ` Sodagudi Prasad
2017-06-15  0:50       ` Sodagudi Prasad
2017-06-15 15:54     ` Mark Rutland
2017-06-15 15:54       ` Mark Rutland
2017-06-19 15:53       ` [PATCH] compiler, clang: Add always_inline attribute to inline Prasad Sodagudi
2017-06-19 15:53         ` Prasad Sodagudi
2017-06-19 20:25         ` David Rientjes
2017-06-19 20:25           ` David Rientjes
2017-06-19 21:14           ` Sodagudi Prasad
2017-06-19 21:14             ` Sodagudi Prasad
2017-06-19 21:42             ` David Rientjes
2017-06-19 21:42               ` David Rientjes
2017-06-19 22:19               ` Sodagudi Prasad
2017-06-19 22:19                 ` Sodagudi Prasad
2017-06-20 10:59                 ` Mark Rutland [this message]
2017-06-20 10:59                   ` Mark Rutland
2017-06-20 23:12                   ` David Rientjes
2017-06-20 23:12                     ` David Rientjes
2017-06-22  9:43                     ` Mark Rutland
2017-06-22  9:43                       ` Mark Rutland
2017-06-23  6:45                       ` Sodagudi Prasad
2017-06-23  6:45                         ` Sodagudi Prasad
2017-06-20 10:52               ` Mark Rutland
2017-06-20 10:52                 ` Mark Rutland

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170620105937.GD28157@leverpostej \
    --to=mark.rutland@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.