llvm dev - Dec 2015 - MSVC warning noise on "LLVM_ATTRIBUTE_ALWAYS_INLINE inline void foo()"

On Mon, Dec 21, 2015 at 12:08 AM, Aaron Ballman <aaron at
aaronballman.com>
wrote:
> On Sun, Dec 20, 2015 at 5:57 PM, Johan Engelen <jbc.engelen at
gmail.com>
> wrote:
> >
> > Perhaps LLVM_ATTRIBUTE_ALWAYS_INLINE could be defined to
"inline" if the
> > compiler has no support for always_inline (currently it is set to
> nothing in
> > that case) ?
> > I think this would allow removal of the "inline" after
> > LLVM_ATTRIBUTE_ALWAYS_INLINE.
>
> Wouldn't this cause functions with MSVC that are marked
> LLVM_ATTRIBUTE_ALWAYS_INLINE but not 'inline' to not be inlined?
>
__forceinline means that MSVC will always inline that function, that is why
the extra "inline" results in a warning.

I propose:
in llvm/Support/Compiler.h

  #if __has_attribute(always_inline) || LLVM_GNUC_PREREQ(4, 0, 0)
#define LLVM_ATTRIBUTE_ALWAYS_INLINE __attribute__((always_inline))
#elif defined(_MSC_VER)  #define LLVM_ATTRIBUTE_ALWAYS_INLINE
__forceinline  #else- #define LLVM_ATTRIBUTE_ALWAYS_INLINE+ #define
LLVM_ATTRIBUTE_ALWAYS_INLINE inline  #endif

and elsewhere

  LLVM_ATTRIBUTE_ALWAYS_INLINE- inline bool operator==(StringRef LHS,
StringRef RHS) {
+ bool operator==(StringRef LHS, StringRef RHS) {
    return LHS.equals(RHS);
  }

As far as I can tell from online documentation, that will do the correct
thing on MSVC and GCC. For non-MSVC, non-GCC, this will add "inline"
in
front of some functions that do not have it right now, notably member
functions that are defined in the class definition (see e.g. StringRef.h
empty()). I will have to test if /that/ doesn't create a warning ;)

 -Johan
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20151221/5c9115df/attachment.html>

On Mon, Dec 21, 2015 at 6:40 AM, Johan Engelen <jbc.engelen at gmail.com>
wrote:
> On Mon, Dec 21, 2015 at 12:08 AM, Aaron Ballman <aaron at
aaronballman.com>
> wrote:
>>
>> On Sun, Dec 20, 2015 at 5:57 PM, Johan Engelen <jbc.engelen at
gmail.com>
>> wrote:
>> >
>> > Perhaps LLVM_ATTRIBUTE_ALWAYS_INLINE could be defined to
"inline" if the
>> > compiler has no support for always_inline (currently it is set to
>> > nothing in
>> > that case) ?
>> > I think this would allow removal of the "inline" after
>> > LLVM_ATTRIBUTE_ALWAYS_INLINE.
>>
>> Wouldn't this cause functions with MSVC that are marked
>> LLVM_ATTRIBUTE_ALWAYS_INLINE but not 'inline' to not be
inlined?
>
>
> __forceinline means that MSVC will always inline that function, that is why
> the extra "inline" results in a warning.
__forceinline is still just a hint to the compiler to do the inline,
but it is a stronger hint that inline
(https://msdn.microsoft.com/en-us/library/bw1hbe6y.aspx). inline is a
keyword in C++ that has additional semantics that __forceinline is not
required to have, such as the fact that an inline function with
external linkage is required to have the same address in all TUs. I am
not comfortable assuming that inline === __forceinline for all
versions of MSVC (including future ones).

~Aaron

>
> I propose:
> in llvm/Support/Compiler.h
>
>   #if __has_attribute(always_inline) || LLVM_GNUC_PREREQ(4, 0, 0)
>   #define LLVM_ATTRIBUTE_ALWAYS_INLINE __attribute__((always_inline))
>   #elif defined(_MSC_VER)
>   #define LLVM_ATTRIBUTE_ALWAYS_INLINE __forceinline
>   #else
> - #define LLVM_ATTRIBUTE_ALWAYS_INLINE
> + #define LLVM_ATTRIBUTE_ALWAYS_INLINE inline
>   #endif
>
> and elsewhere
>
>   LLVM_ATTRIBUTE_ALWAYS_INLINE
> - inline bool operator==(StringRef LHS, StringRef RHS) {
> + bool operator==(StringRef LHS, StringRef RHS) {
>     return LHS.equals(RHS);
>   }
>
> As far as I can tell from online documentation, that will do the correct
> thing on MSVC and GCC. For non-MSVC, non-GCC, this will add
"inline" in
> front of some functions that do not have it right now, notably member
> functions that are defined in the class definition (see e.g. StringRef.h
> empty()). I will have to test if /that/ doesn't create a warning ;)
>
>  -Johan
>
>

OK, thanks for the extra explanation.
I guess LDC will have to go the same route of silencing C4141 warnings (
http://llvm.org/viewvc/llvm-project?view=revision&revision=247275).

(sorry for the maillist noise ;)

- Johan


On Mon, Dec 21, 2015 at 3:57 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:
> On Mon, Dec 21, 2015 at 6:40 AM, Johan Engelen <jbc.engelen at
gmail.com>
> wrote:
> > On Mon, Dec 21, 2015 at 12:08 AM, Aaron Ballman <aaron at
aaronballman.com>
> > wrote:
> >>
> >> On Sun, Dec 20, 2015 at 5:57 PM, Johan Engelen <jbc.engelen at
gmail.com>
> >> wrote:
> >> >
> >> > Perhaps LLVM_ATTRIBUTE_ALWAYS_INLINE could be defined to
"inline" if
> the
> >> > compiler has no support for always_inline (currently it is
set to
> >> > nothing in
> >> > that case) ?
> >> > I think this would allow removal of the "inline"
after
> >> > LLVM_ATTRIBUTE_ALWAYS_INLINE.
> >>
> >> Wouldn't this cause functions with MSVC that are marked
> >> LLVM_ATTRIBUTE_ALWAYS_INLINE but not 'inline' to not be
inlined?
> >
> >
> > __forceinline means that MSVC will always inline that function, that
is
> why
> > the extra "inline" results in a warning.
>
> __forceinline is still just a hint to the compiler to do the inline,
> but it is a stronger hint that inline
> (https://msdn.microsoft.com/en-us/library/bw1hbe6y.aspx). inline is a
> keyword in C++ that has additional semantics that __forceinline is not
> required to have, such as the fact that an inline function with
> external linkage is required to have the same address in all TUs. I am
> not comfortable assuming that inline === __forceinline for all
> versions of MSVC (including future ones).
>
> ~Aaron
>
>
> >
> > I propose:
> > in llvm/Support/Compiler.h
> >
> >   #if __has_attribute(always_inline) || LLVM_GNUC_PREREQ(4, 0, 0)
> >   #define LLVM_ATTRIBUTE_ALWAYS_INLINE __attribute__((always_inline))
> >   #elif defined(_MSC_VER)
> >   #define LLVM_ATTRIBUTE_ALWAYS_INLINE __forceinline
> >   #else
> > - #define LLVM_ATTRIBUTE_ALWAYS_INLINE
> > + #define LLVM_ATTRIBUTE_ALWAYS_INLINE inline
> >   #endif
> >
> > and elsewhere
> >
> >   LLVM_ATTRIBUTE_ALWAYS_INLINE
> > - inline bool operator==(StringRef LHS, StringRef RHS) {
> > + bool operator==(StringRef LHS, StringRef RHS) {
> >     return LHS.equals(RHS);
> >   }
> >
> > As far as I can tell from online documentation, that will do the
correct
> > thing on MSVC and GCC. For non-MSVC, non-GCC, this will add
"inline" in
> > front of some functions that do not have it right now, notably member
> > functions that are defined in the class definition (see e.g.
StringRef.h
> > empty()). I will have to test if /that/ doesn't create a warning
;)
> >
> >  -Johan
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20151221/d409a1b3/attachment-0001.html>