Using __always_inline attribute

Using __always_inline attribute

[Sodagudi Prasad]

Hi All,

With a variant of a CLANG(based on 4.0) following errors observed on 
Linux 4.12-rc5 tag.

net/built-in.o: In function `__xchg_mb':
arch/arm64/include/asm/cmpxchg.h:99: \
                     undefined reference to `__compiletime_assert_99'
arch/arm64/include/asm/cmpxchg.h:99: \
                     undefined reference to `__compiletime_assert_99

Clang does not seems to be marking this macro as inline and causing 
above compilation issue due to BUILD_BUG().

We added  __always_inline attribute to this macro as shown below, so 
that clang forces this macro to be always inline.
Based on definition of __xchg##sfx, it should always be inline.  Can we 
force this macro to be __always_inline ?


diff --git a/arch/arm64/include/asm/cmpxchg.h 
b/arch/arm64/include/asm/cmpxchg.h
index ae852ad..ce57cec 100644
--- a/arch/arm64/include/asm/cmpxchg.h
+++ b/arch/arm64/include/asm/cmpxchg.h
@@ -73,7 +73,7 @@
  #undef __XCHG_CASE

  #define __XCHG_GEN(sfx)                                                 
        \
-static inline unsigned long __xchg##sfx(unsigned long x,               
\
+static __always_inline unsigned long __xchg##sfx(unsigned long x,       
        \
                                         volatile void *ptr,             
\
                                         int size)                       
\
  {                                                                      
\


-Thanks, Prasad

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: Using __always_inline attribute

[Will Deacon]

Hi Prasad,

On Tue, Jun 13, 2017 at 03:39:37PM -0700, Sodagudi Prasad wrote:
> With a variant of a CLANG(based on 4.0) following errors observed on Linux
> 4.12-rc5 tag.

How are you building the kernel with clang, btw? I'd be keen to try the same
thing, but I thought the LLVMLinux project was largely dead at his point. Do
you still need build system patches, or is this now integrated with Kbuild?

> net/built-in.o: In function `__xchg_mb':
> arch/arm64/include/asm/cmpxchg.h:99: \
>                     undefined reference to `__compiletime_assert_99'
> arch/arm64/include/asm/cmpxchg.h:99: \
>                     undefined reference to `__compiletime_assert_99
> 
> Clang does not seems to be marking this macro as inline and causing above
> compilation issue due to BUILD_BUG().

The only BUILD_BUG I see around here is if the size parameter (which is
calculated using sizeof) is not known to be 1,2,4 or 8 at compile time. It
would be interesting to know which call site is confusing clang in this way
and what code is actually being emitted here.

If it's just that __xchg_mb isn't being inlined into the __xchg_wrapper
macro, then your patch should be ok, but I'd like to be sure it's not
something else. I'm also surprised you haven't see this crop up in other
places.

Will

Re: Using __always_inline attribute

[Sodagudi Prasad]

On 2017-06-14 03:06, Will Deacon wrote:
> Hi Prasad,
> 
> On Tue, Jun 13, 2017 at 03:39:37PM -0700, Sodagudi Prasad wrote:
>> With a variant of a CLANG(based on 4.0) following errors observed on 
>> Linux
>> 4.12-rc5 tag.
> 
> How are you building the kernel with clang, btw? I'd be keen to try the 
> same
> thing, but I thought the LLVMLinux project was largely dead at his 
> point. Do
> you still need build system patches, or is this now integrated with 
> Kbuild?
Hi Will,
Trying clang patches from below tree -
https://android.googlesource.com/kernel/common/+/experimental/android-4.4-llvm

> 
>> net/built-in.o: In function `__xchg_mb':
>> arch/arm64/include/asm/cmpxchg.h:99: \
>>                     undefined reference to `__compiletime_assert_99'
>> arch/arm64/include/asm/cmpxchg.h:99: \
>>                     undefined reference to `__compiletime_assert_99
>> 
>> Clang does not seems to be marking this macro as inline and causing 
>> above
>> compilation issue due to BUILD_BUG().
> 
> The only BUILD_BUG I see around here is if the size parameter (which is
> calculated using sizeof) is not known to be 1,2,4 or 8 at compile time. 
> It
> would be interesting to know which call site is confusing clang in this 
> way
> and what code is actually being emitted here.
> 
> If it's just that __xchg_mb isn't being inlined into the __xchg_wrapper
> macro, then your patch should be ok, but I'd like to be sure it's not
> something else. I'm also surprised you haven't see this crop up in 
> other
> places.
After digging further, we observed that inline definition was changed 
recently and causing this issue.
Here is missing part of inline macro definition __attribute__((unused)).

Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
static inline functions") have redefined the inline macro as below
#define inline inline __attribute__((unused))

But actual definition of inline macro defined compiler-gcc.h file as 
shown below.
#define inline          inline          __attribute__((always_inline)) 
notrace

As always_inline attribute is missing in inline definition, compile may 
not inline macros __xchg_mb in
arch/arm64/include/asm/cmpxchg.h file and leading to error.

Some compilers may not honor inline as inline if always_inline attribute 
is removed because of
-inline-threshold compiler options.

Here is the change to fix this issue-
diff --git a/include/linux/compiler-clang.h 
b/include/linux/compiler-clang.h
index d614c5e..a0e6433 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -22,4 +22,4 @@
   * directives.  Suppress the warning in clang as well.
   */
  #undef inline
-#define inline inline __attribute__((unused)) notrace
+#define inline __attribute__((always_inline)) __attribute__((unused)) 
notrace

-Thanks, Prasad
> 
> Will

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: Using __always_inline attribute

[Sodagudi Prasad]

On 2017-06-14 15:33, Sodagudi Prasad wrote:
> On 2017-06-14 03:06, Will Deacon wrote:
>> Hi Prasad,
>> 
>> On Tue, Jun 13, 2017 at 03:39:37PM -0700, Sodagudi Prasad wrote:
>>> With a variant of a CLANG(based on 4.0) following errors observed on 
>>> Linux
>>> 4.12-rc5 tag.
>> 
>> How are you building the kernel with clang, btw? I'd be keen to try 
>> the same
>> thing, but I thought the LLVMLinux project was largely dead at his 
>> point. Do
>> you still need build system patches, or is this now integrated with 
>> Kbuild?
> Hi Will,
> Trying clang patches from below tree -
> https://android.googlesource.com/kernel/common/+/experimental/android-4.4-llvm
> 
>> 
>>> net/built-in.o: In function `__xchg_mb':
>>> arch/arm64/include/asm/cmpxchg.h:99: \
>>>                     undefined reference to `__compiletime_assert_99'
>>> arch/arm64/include/asm/cmpxchg.h:99: \
>>>                     undefined reference to `__compiletime_assert_99
>>> 
>>> Clang does not seems to be marking this macro as inline and causing 
>>> above
>>> compilation issue due to BUILD_BUG().
>> 
>> The only BUILD_BUG I see around here is if the size parameter (which 
>> is
>> calculated using sizeof) is not known to be 1,2,4 or 8 at compile 
>> time. It
>> would be interesting to know which call site is confusing clang in 
>> this way
>> and what code is actually being emitted here.
>> 
>> If it's just that __xchg_mb isn't being inlined into the 
>> __xchg_wrapper
>> macro, then your patch should be ok, but I'd like to be sure it's not
>> something else. I'm also surprised you haven't see this crop up in 
>> other
>> places.
> After digging further, we observed that inline definition was changed
> recently and causing this issue.
> Here is missing part of inline macro definition 
> __attribute__((unused)).
> 
> Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
> static inline functions") have redefined the inline macro as below
> #define inline inline __attribute__((unused))
> 
> But actual definition of inline macro defined compiler-gcc.h file as
> shown below.
> #define inline          inline          __attribute__((always_inline)) 
> notrace
> 
> As always_inline attribute is missing in inline definition, compile
> may not inline macros __xchg_mb in
> arch/arm64/include/asm/cmpxchg.h file and leading to error.
> 
> Some compilers may not honor inline as inline if always_inline
> attribute is removed because of
> -inline-threshold compiler options.
> 
> Here is the change to fix this issue-
> diff --git a/include/linux/compiler-clang.h 
> b/include/linux/compiler-clang.h
> index d614c5e..a0e6433 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -22,4 +22,4 @@
>   * directives.  Suppress the warning in clang as well.
>   */
>  #undef inline
> -#define inline inline __attribute__((unused)) notrace
> +#define inline __attribute__((always_inline)) __attribute__((unused)) 
> notrace
> 
> -Thanks, Prasad

Hi Will,

Here is the proper patch -
     compiler, clang: Add always_inline attribute to inline

     Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
     static inline functions") re-defining the 'inline' macro but
     __attribute__((always_inline)) is missing. Some compilers may
     not honor inline hint if always_iniline attribute is not there.
     So add always_inline attribute to inline as done by
     compiler-gcc.h file.

diff --git a/include/linux/compiler-clang.h 
b/include/linux/compiler-clang.h
index d614c5e..400b0cd 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -22,4 +22,4 @@
   * directives.  Suppress the warning in clang as well.
   */
  #undef inline
-#define inline inline __attribute__((unused)) notrace
+#define inline inline __attribute__((always_inline)) 
__attribute__((unused)) notrace

-Thanks, Prasad

>> 
>> Will

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: Using __always_inline attribute

[Mark Rutland]

Hi,

On Wed, Jun 14, 2017 at 03:33:59PM -0700, Sodagudi Prasad wrote:
> On 2017-06-14 03:06, Will Deacon wrote:
> >On Tue, Jun 13, 2017 at 03:39:37PM -0700, Sodagudi Prasad wrote:
> >>With a variant of a CLANG(based on 4.0) following errors
> >>observed on Linux
> >>4.12-rc5 tag.

> >>net/built-in.o: In function `__xchg_mb':
> >>arch/arm64/include/asm/cmpxchg.h:99: \
> >>                    undefined reference to `__compiletime_assert_99'
> >>arch/arm64/include/asm/cmpxchg.h:99: \
> >>                    undefined reference to `__compiletime_assert_99
> >>
> >>Clang does not seems to be marking this macro as inline and
> >>causing above
> >>compilation issue due to BUILD_BUG().
> >
> >The only BUILD_BUG I see around here is if the size parameter (which
> >is calculated using sizeof) is not known to be 1,2,4 or 8 at compile
> >time. It would be interesting to know which call site is confusing
> >clang in this way and what code is actually being emitted here.
> >
> >If it's just that __xchg_mb isn't being inlined into the
> >__xchg_wrapper macro, then your patch should be ok, but I'd like to
> >be sure it's not something else. I'm also surprised you haven't see
> >this crop up in other places.
> >
> After digging further, we observed that inline definition was
> changed recently and causing this issue.
> Here is missing part of inline macro definition __attribute__((unused)).
> 
> Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
> static inline functions") have redefined the inline macro as below
> #define inline inline __attribute__((unused))
> 
> But actual definition of inline macro defined compiler-gcc.h file as
> shown below.
> #define inline          inline
> __attribute__((always_inline)) notrace

FWIW, this happesn to be true for arm64 today, but it's not always the
case. When ARCH_SUPPORTS_OPTIMIZED_INLINING && OPTIMIZE_INLINING, inline
is not equivalent to __always_inline.

It looks like x86 has ARCH_SUPPORTS_OPTIMIZED_INLINING and selects
OPTIMIZE_INLINING in its defconfigs, so core code should be clean, and
presumably the option is a net win on x86.

It mgiht be worth taking a look if this would be beneficial for arm64,
even if we have to apply a few s/inline/__always_inline/ fixups as with
this case.

> As always_inline attribute is missing in inline definition, compile
> may not inline macros __xchg_mb in
> arch/arm64/include/asm/cmpxchg.h file and leading to error.
> 
> Some compilers may not honor inline as inline if always_inline
> attribute is removed because of
> -inline-threshold compiler options.
> 
> Here is the change to fix this issue-
> diff --git a/include/linux/compiler-clang.h
> b/include/linux/compiler-clang.h
> index d614c5e..a0e6433 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -22,4 +22,4 @@
>   * directives.  Suppress the warning in clang as well.
>   */
>  #undef inline
> -#define inline inline __attribute__((unused)) notrace
> +#define inline __attribute__((always_inline))
> __attribute__((unused)) notrace

Assuming this has the same gaurds for ARCH_SUPPORTS_OPTIMIZED_INLINING
&& OPTIMIZE_INLINING, it looks fine to me. Otherwise, I suspect this may
be overly pessimistic.

Thanks,
Mark.

[PATCH] compiler, clang: Add always_inline attribute to inline

[Prasad Sodagudi]

Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
static inline functions") re-defining the 'inline' macro but
__attribute__((always_inline)) is missing. Some compilers may
not honor inline hint if always_iniline attribute not there.
So add always_inline attribute to inline as done by
compiler-gcc.h file.

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
---
 include/linux/compiler-clang.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index d614c5e..deb65b3 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -22,4 +22,9 @@
  * directives.  Suppress the warning in clang as well.
  */
 #undef inline
+#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
+    !defined(CONFIG_OPTIMIZE_INLINING)
+#define inline inline __attribute__((always_inline)) __attribute__((unused)) notrace
+#else
 #define inline inline __attribute__((unused)) notrace
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[David Rientjes]

On Mon, 19 Jun 2017, Prasad Sodagudi wrote:

> Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
> static inline functions") re-defining the 'inline' macro but
> __attribute__((always_inline)) is missing. Some compilers may
> not honor inline hint if always_iniline attribute not there.
> So add always_inline attribute to inline as done by
> compiler-gcc.h file.
> 

IIUC, __attribute__((always_inline)) was only needed for gcc versions < 4 
and that the inlining decision making is improved in >= 4.  To make a 
change like this, I would think that we would need to show that clang is 
making suboptimal decisions.  I don't think there's a downside to making 
CONFIG_OPTIMIZE_INLINING specific only to gcc.

If it is shown that __attribute__((always_inline)) is needed for clang as 
well, this should be done as part of compiler-gcc.h to avoid duplicated 
code.

> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> ---
>  include/linux/compiler-clang.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index d614c5e..deb65b3 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -22,4 +22,9 @@
>   * directives.  Suppress the warning in clang as well.
>   */
>  #undef inline
> +#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
> +    !defined(CONFIG_OPTIMIZE_INLINING)
> +#define inline inline __attribute__((always_inline)) __attribute__((unused)) notrace
> +#else
>  #define inline inline __attribute__((unused)) notrace
> +#endif

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[Sodagudi Prasad]

On 2017-06-19 13:25, David Rientjes wrote:
> On Mon, 19 Jun 2017, Prasad Sodagudi wrote:
> 
>> Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
>> static inline functions") re-defining the 'inline' macro but
>> __attribute__((always_inline)) is missing. Some compilers may
>> not honor inline hint if always_iniline attribute not there.
>> So add always_inline attribute to inline as done by
>> compiler-gcc.h file.
>> 
> 
> IIUC, __attribute__((always_inline)) was only needed for gcc versions < 
> 4
> and that the inlining decision making is improved in >= 4.  To make a
> change like this, I would think that we would need to show that clang 
> is
> making suboptimal decisions.  I don't think there's a downside to 
> making
> CONFIG_OPTIMIZE_INLINING specific only to gcc.
> 
> If it is shown that __attribute__((always_inline)) is needed for clang 
> as
> well, this should be done as part of compiler-gcc.h to avoid duplicated
> code.

Hi David,

Here is the discussion about this change -  
https://lkml.org/lkml/2017/6/15/396
Please check mark and will's comments.

-Thanks, Prasad

> 
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> ---
>>  include/linux/compiler-clang.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/include/linux/compiler-clang.h 
>> b/include/linux/compiler-clang.h
>> index d614c5e..deb65b3 100644
>> --- a/include/linux/compiler-clang.h
>> +++ b/include/linux/compiler-clang.h
>> @@ -22,4 +22,9 @@
>>   * directives.  Suppress the warning in clang as well.
>>   */
>>  #undef inline
>> +#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
>> +    !defined(CONFIG_OPTIMIZE_INLINING)
>> +#define inline inline __attribute__((always_inline)) 
>> __attribute__((unused)) notrace
>> +#else
>>  #define inline inline __attribute__((unused)) notrace
>> +#endif

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[David Rientjes]

On Mon, 19 Jun 2017, Sodagudi Prasad wrote:

> > > Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
> > > static inline functions") re-defining the 'inline' macro but
> > > __attribute__((always_inline)) is missing. Some compilers may
> > > not honor inline hint if always_iniline attribute not there.
> > > So add always_inline attribute to inline as done by
> > > compiler-gcc.h file.
> > > 
> > 
> > IIUC, __attribute__((always_inline)) was only needed for gcc versions < 4
> > and that the inlining decision making is improved in >= 4.  To make a
> > change like this, I would think that we would need to show that clang is
> > making suboptimal decisions.  I don't think there's a downside to making
> > CONFIG_OPTIMIZE_INLINING specific only to gcc.
> > 
> > If it is shown that __attribute__((always_inline)) is needed for clang as
> > well, this should be done as part of compiler-gcc.h to avoid duplicated
> > code.
> 
> Hi David,
> 
> Here is the discussion about this change -
> https://lkml.org/lkml/2017/6/15/396
> Please check mark and will's comments.
> 

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.

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[Sodagudi Prasad]

On 2017-06-19 14:42, David Rientjes wrote:
> On Mon, 19 Jun 2017, Sodagudi Prasad wrote:
> 
>> > > Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
>> > > static inline functions") re-defining the 'inline' macro but
>> > > __attribute__((always_inline)) is missing. Some compilers may
>> > > not honor inline hint if always_iniline attribute not there.
>> > > So add always_inline attribute to inline as done by
>> > > compiler-gcc.h file.
>> > >
>> >
>> > IIUC, __attribute__((always_inline)) was only needed for gcc versions < 4
>> > and that the inlining decision making is improved in >= 4.  To make a
>> > change like this, I would think that we would need to show that clang is
>> > making suboptimal decisions.  I don't think there's a downside to making
>> > CONFIG_OPTIMIZE_INLINING specific only to gcc.
>> >
>> > If it is shown that __attribute__((always_inline)) is needed for clang as
>> > well, this should be done as part of compiler-gcc.h to avoid duplicated
>> > code.
>> 
>> Hi David,
>> 
>> Here is the discussion about this change -
>> https://lkml.org/lkml/2017/6/15/396
>> Please check mark and will's comments.
>> 
> 
> 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.

-Thanks, Prasad

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[Mark Rutland]

On Mon, Jun 19, 2017 at 02:42:23PM -0700, David Rientjes wrote:
> On Mon, 19 Jun 2017, Sodagudi Prasad wrote:
> 
> > > > Commit abb2ea7dfd82 ("compiler, clang: suppress warning for unused
> > > > static inline functions") re-defining the 'inline' macro but
> > > > __attribute__((always_inline)) is missing. Some compilers may
> > > > not honor inline hint if always_iniline attribute not there.
> > > > So add always_inline attribute to inline as done by
> > > > compiler-gcc.h file.
> > > > 
> > > 
> > > IIUC, __attribute__((always_inline)) was only needed for gcc versions < 4
> > > and that the inlining decision making is improved in >= 4.  To make a
> > > change like this, I would think that we would need to show that clang is
> > > making suboptimal decisions.  I don't think there's a downside to making
> > > CONFIG_OPTIMIZE_INLINING specific only to gcc.
> > > 
> > > If it is shown that __attribute__((always_inline)) is needed for clang as
> > > well, this should be done as part of compiler-gcc.h to avoid duplicated
> > > code.
> > 
> > Hi David,
> > 
> > Here is the discussion about this change -
> > https://lkml.org/lkml/2017/6/15/396
> > Please check mark and will's comments.
> > 
> 
> 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.

My concern here is that code has been written with the implicit
assumption that inline means __always_inline, since that's been the case
for years with GCC, when !ARCH_SUPPORTS_OPTIMIZED_INLINING ||
!CONFIG_OPTIMIZE_INLINING, (i.e. for every !x86 arch).

While this is the only breakage seen so far, it seems likely that
similar breakage may exist elsewhere, and such breakage may easily be
introduced by those only using GCC.

I'd prefer to use the same guards for clang here, since that ensures
that such code works by default across both compilers. That gives us the
chance to test and fixup code without a violent flag day.

Once we've fixed up the core arm64 code, we can select
ARCH_SUPPORTS_OPTIMIZED_INLINING, and allow users to optionally select
CONFIG_OPTIMIZE_INLINING (with either compiler).

Once that's seen some testing, and if there's a benefit, then we can try
to align with x86 and default to selecting CONFIG_OPTIMIZE_INLINING,
and/or drop the config options entirely and only check the GCC version.

Thanks,
Mark.

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[Mark Rutland]

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.

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[David Rientjes]

On Tue, 20 Jun 2017, Mark Rutland wrote:

> 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.
> 

Sounds good, but I think we should simply deal with the 
__attribute__((unused)) needed for clang as part of compiler-gcc.h by 
simply adding it to the inline override there to avoid duplicated code.

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -15,11 +15,3 @@
  * with any version that can compile the kernel
  */
 #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
-
-/*
- * GCC does not warn about unused static inline functions for
- * -Wunused-function.  This turns out to avoid the need for complex #ifdef
- * directives.  Suppress the warning in clang as well.
- */
-#undef inline
-#define inline inline __attribute__((unused)) notrace
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 0efef9cf014f..71fe0994cf1a 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -66,18 +66,22 @@
 
 /*
  * Force always-inline if the user requests it so via the .config,
- * or if gcc is too old:
+ * or if gcc is too old.
+ * GCC does not warn about unused static inline functions for
+ * -Wunused-function.  This turns out to avoid the need for complex #ifdef
+ * directives.  Suppress the warning in clang as well by using "unused"
+ * function attribute, which is redundant but not harmful for gcc.
  */
 #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
     !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
-#define inline		inline		__attribute__((always_inline)) notrace
-#define __inline__	__inline__	__attribute__((always_inline)) notrace
-#define __inline	__inline	__attribute__((always_inline)) notrace
+#define inline inline		__attribute__((always_inline,unused)) notrace
+#define __inline__ __inline__	__attribute__((always_inline,unused)) notrace
+#define __inline __inline	__attribute__((always_inline,unused)) notrace
 #else
 /* A lot of inline functions can cause havoc with function tracing */
-#define inline		inline		notrace
-#define __inline__	__inline__	notrace
-#define __inline	__inline	notrace
+#define inline inline		__attribute__((unused)) notrace
+#define __inline__ __inline__	__attribute__((unused)) notrace
+#define __inline __inline	__attribute__((unused)) notrace
 #endif
 
 #define __always_inline	inline __attribute__((always_inline))

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[Mark Rutland]

On Tue, Jun 20, 2017 at 04:12:32PM -0700, David Rientjes wrote:
> On Tue, 20 Jun 2017, Mark Rutland wrote:
> 
> > 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.
> > 
> 
> Sounds good, but I think we should simply deal with the 
> __attribute__((unused)) needed for clang as part of compiler-gcc.h by 
> simply adding it to the inline override there to avoid duplicated code.

Agreed. That looks much better.

Thanks,
Mark.

> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -15,11 +15,3 @@
>   * with any version that can compile the kernel
>   */
>  #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
> -
> -/*
> - * GCC does not warn about unused static inline functions for
> - * -Wunused-function.  This turns out to avoid the need for complex #ifdef
> - * directives.  Suppress the warning in clang as well.
> - */
> -#undef inline
> -#define inline inline __attribute__((unused)) notrace
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 0efef9cf014f..71fe0994cf1a 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -66,18 +66,22 @@
>  
>  /*
>   * Force always-inline if the user requests it so via the .config,
> - * or if gcc is too old:
> + * or if gcc is too old.
> + * GCC does not warn about unused static inline functions for
> + * -Wunused-function.  This turns out to avoid the need for complex #ifdef
> + * directives.  Suppress the warning in clang as well by using "unused"
> + * function attribute, which is redundant but not harmful for gcc.
>   */
>  #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
>      !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
> -#define inline		inline		__attribute__((always_inline)) notrace
> -#define __inline__	__inline__	__attribute__((always_inline)) notrace
> -#define __inline	__inline	__attribute__((always_inline)) notrace
> +#define inline inline		__attribute__((always_inline,unused)) notrace
> +#define __inline__ __inline__	__attribute__((always_inline,unused)) notrace
> +#define __inline __inline	__attribute__((always_inline,unused)) notrace
>  #else
>  /* A lot of inline functions can cause havoc with function tracing */
> -#define inline		inline		notrace
> -#define __inline__	__inline__	notrace
> -#define __inline	__inline	notrace
> +#define inline inline		__attribute__((unused)) notrace
> +#define __inline__ __inline__	__attribute__((unused)) notrace
> +#define __inline __inline	__attribute__((unused)) notrace
>  #endif
>  
>  #define __always_inline	inline __attribute__((always_inline))

Re: [PATCH] compiler, clang: Add always_inline attribute to inline

[Sodagudi Prasad]

On 2017-06-22 02:43, Mark Rutland wrote:
> On Tue, Jun 20, 2017 at 04:12:32PM -0700, David Rientjes wrote:
>> On Tue, 20 Jun 2017, Mark Rutland wrote:
>> 
>> > 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.
>> >
>> 
>> Sounds good, but I think we should simply deal with the
>> __attribute__((unused)) needed for clang as part of compiler-gcc.h by
>> simply adding it to the inline override there to avoid duplicated 
>> code.
> 
> Agreed. That looks much better.
> 
> Thanks,
> Mark.
> 
>> 
>> diff --git a/include/linux/compiler-clang.h 
>> b/include/linux/compiler-clang.h
>> --- a/include/linux/compiler-clang.h
>> +++ b/include/linux/compiler-clang.h
>> @@ -15,11 +15,3 @@
>>   * with any version that can compile the kernel
>>   */
>>  #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), 
>> __COUNTER__)
>> -
>> -/*
>> - * GCC does not warn about unused static inline functions for
>> - * -Wunused-function.  This turns out to avoid the need for complex 
>> #ifdef
>> - * directives.  Suppress the warning in clang as well.
>> - */
>> -#undef inline
>> -#define inline inline __attribute__((unused)) notrace
>> diff --git a/include/linux/compiler-gcc.h 
>> b/include/linux/compiler-gcc.h
>> index 0efef9cf014f..71fe0994cf1a 100644
>> --- a/include/linux/compiler-gcc.h
>> +++ b/include/linux/compiler-gcc.h
>> @@ -66,18 +66,22 @@
>> 
>>  /*
>>   * Force always-inline if the user requests it so via the .config,
>> - * or if gcc is too old:
>> + * or if gcc is too old.
>> + * GCC does not warn about unused static inline functions for
>> + * -Wunused-function.  This turns out to avoid the need for complex 
>> #ifdef
>> + * directives.  Suppress the warning in clang as well by using 
>> "unused"
>> + * function attribute, which is redundant but not harmful for gcc.
>>   */
>>  #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
>>      !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
>> -#define inline		inline		__attribute__((always_inline)) notrace
>> -#define __inline__	__inline__	__attribute__((always_inline)) notrace
>> -#define __inline	__inline	__attribute__((always_inline)) notrace
>> +#define inline inline		__attribute__((always_inline,unused)) notrace
>> +#define __inline__ __inline__	__attribute__((always_inline,unused)) 
>> notrace
>> +#define __inline __inline	__attribute__((always_inline,unused)) 
>> notrace
>>  #else
>>  /* A lot of inline functions can cause havoc with function tracing */
>> -#define inline		inline		notrace
>> -#define __inline__	__inline__	notrace
>> -#define __inline	__inline	notrace
>> +#define inline inline		__attribute__((unused)) notrace
>> +#define __inline__ __inline__	__attribute__((unused)) notrace
>> +#define __inline __inline	__attribute__((unused)) notrace
>>  #endif
>> 
>>  #define __always_inline	inline __attribute__((always_inline))
Hi David,

I just want to cross check with you.
Do you want me to update this change in next patch set ? Or are you 
going to add this ?

-Thanks, Prasad
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project