Pass inlinehint attribute to optimizer for all user marked inline functions

[bmakam]

Summary

With inline member functions that are defined outside the class, clang currently passes inlinehint attribute to optimizer if inline keyword is placed next to the declaration within the class body. In real world inline keyword is placed next to definition outside the class body because the public part of the class body is observable semantics and practically placing it next to definition makes it easier and safer for class's reusers. The decision of whether to inline or not to inline shouldn't change the observable semantics of the call.
Here is an example of an inline member function defined outside the class body:

class Foo {
public:
  void method();  // Best practice: Don't put the inline keyword here
  // ...
};
inline void Foo::method()  // Best practice: Put the inline keyword here
{
  // ...
}

This patch fixes the above issue by passing inlinehint attribute if the function can be inherently inlined.

[mcrosier]

I've added Doug and John in hopes that someone with more experience with this code can give a proper review. Jakob was the original author..

AFAICT, the isInlined() API will return true for more than just those decls that include the 'inline' keyword. I'm not sure that's the behavior we want. I wonder if we're correctly setting the IsInlineSpecified variable when the redecl is instantiated?

[bmakam]

In D8656#147887, @mcrosier wrote:

I've added Doug and John in hopes that someone with more experience with this code can give a proper review. Jakob was the original author..

AFAICT, the isInlined() API will return true for more than just those decls that include the 'inline' keyword. I'm not sure that's the behavior we want. I wonder if we're correctly setting the IsInlineSpecified variable when the redecl is instantiated?

isInlined() API will return true for functions that are implicitly inlined because it is either marked inline, or constexpr or is a member function of a class that was defined in the class body. isInlineSpecified() API determines whether inline keyword was specified for this function. In this example, isInlineSpecified() returns false because inline keyword was specified outside the class body. isInlineSpecified variable is correctly set if the inline keyword was specified inside the class body, so I think instantiating redecl is correctly setting the isInlineSpecified variable.
Are you saying we need to correctly set isInlineSpecified variable if the inline keyword was specified even outside of the class body? I'm not sure if isInlineSpecified() API was designed to handle this case and think isInlined() API was designed to handle such cases. I will wait for Doug and John to weigh in with their opinion.

[bmakam]

After discussing this over irc, there is a general feeling that this may not be the right fix. So, I am abandoning this change. I will look into further on how to pass inlinehint to optimizer for members user marked as inline outside the class.

[rnk]

This looks like a bug in template instantiation. I have a patch-in-progress
that transfers the inline specifier from the pattern to the instantiation.

[bmakam]

In D8656#150487, @rnk wrote:

This looks like a bug in template instantiation. I have a patch-in-progress
that transfers the inline specifier from the pattern to the instantiation.

Thanks. I have modified my original patch a little bit to do the same. I will put it up for review shortly.

[rnk]

I went ahead and committed mine in r233817, since the consensus here was
that this was a minor template instantiation bug.

[bmakam]

In D8656#150583, @rnk wrote:

I went ahead and committed mine in r233817, since the consensus here was
that this was a minor template instantiation bug.

Cool. Thanks!

[mcrosier]

Many thanks, Reed!

lib/CodeGen/CodeGenFunction.cpp

if (CGM.isInSanitizerBlacklist(Fn, Loc))
     SanOpts.clear();
 
   // Pass inline keyword to optimizer if it appears explicitly on any
   // declaration. Also, in the case of -fno-inline attach NoInline
   // attribute to all function that are not marked AlwaysInline.
   if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) {
     if (!CGM.getCodeGenOpts().NoInline) {
       for (auto RI : FD->redecls())
-        if (RI->isInlineSpecified()) {
+        if (RI->isInlined()) {
           Fn->addFnAttr(llvm::Attribute::InlineHint);
           break;
         }
     } else if (!FD->hasAttr<AlwaysInlineAttr>())
       Fn->addFnAttr(llvm::Attribute::NoInline);
   }
 
   if (getLangOpts().OpenCL) {

test/CodeGen/inlinehintattr.cpp

+// RUN: %clang_cc1 %s -disable-llvm-optzns -emit-llvm -o- | FileCheck %s --check-prefix=INLINE
+// INLINE-LABEL: Function Attrs: inlinehint
+// INLINE-LABEL-NEXT: @_ZN3fooIiE3addEv
+
+template <class T> class foo {
+  public:
+    T* tp;
+    int elem;
+    int maxelem;
+    T* add();
+};
+
+template <class T> inline T* foo <T>::add()
+{
+  if (elem!=maxelem)
+    elem++;
+  return tp+elem-1;
+}
+
+typedef foo <int> intboundart;
+
+class bar
+{
+  void addtobound(intboundart& boundarp);
+};
+
+void bar::addtobound(intboundart& barp)
+{
+  barp.add();
+}