[modules] Defer emission of inline key functions.

[v.g.vassilev]

Summary

If the ABI supports inline key functions we must emit them eagerly. The generic reason is that CodeGen might assume they are emitted and generate a reference to the vtable. In deserialization this rule becomes a little less clear because if a reference is generated, CodeGen will make a request and we will emit the vtable on demand.

This patch improves the performance of loading modules by slimming down the EAGERLY_DESERIALIZED_DECLS sections. It is equivalent to 'pinning' the vtable, with two benefits: (a) it does not require expertise in modules to find out that outlining the key function brings performance; (b) it also helps with third-party codebases.

We ran this change through our codebase and we saw no observable changes in behavior.

Patch by Yuka Takahashi and me!

[v.g.vassilev]

@rsmith, @bruno could you verify this patch on your codebases?

[rjmccall]

Seems reasonable to me.

[rsmith]

If CodeGen is able to emit inline key functions when the vtable is needed anyway, can we instead change DeclMustBeEmitted to not treat inline key functions specially? There is no reason we need to emit any symbols for:

struct A { virtual void f(); };
inline void A::f() {}

... and if suppressing DeclMustBeEmitted works in the modules case, it really should work in the general case.

Indeed, deleting the relevant code from DeclMustBeEmitted appears to work fine. It's not actually enough to prevent us from spuriously emitting the vtable and inline function definition in the above case, though: Sema marks the vtable as "used" because the key function was defined (it doesn't know whether CodeGen might decide to emit it and prepares for the worst, but that then marks the vtable as "used", which *forces* it to be emitted). Fixing that requires some more invasive changes.

See https://reviews.llvm.org/D54986 for my proposed alternative to this. The idea is to allow CodeGen to decide when to emit a vtable, and to do so only when we actually emit a use of the vtable / key function / explicit instantiation definition of the class. This doesn't require any eagerly-deserialized declaration tracking except for explicit instantiation definitions.

lib/Serialization/ASTWriterDecl.cpp

2267–2278

If the function must be emitted for some reason other than being a key function (for instance, due to __attribute__((used)) or simply not being declared inline), this may result in it not getting emitted; that doesn't seem correct. (For a module it might typically work because most such cases would be redefinition errors, but it's wrong for cases like a precompiled preamble or PCH, which can be intended to be used only once and thus can contain strong external symbol definitions.)

[v.g.vassilev]

Landed as D54986

include/clang/AST/ASTContext.h

if (!std::is_trivially_destructible<T>::value) {
       auto DestroyPtr = [](void *V) { static_cast<T *>(V)->~T(); };
       AddDeallocation(DestroyPtr, Ptr);
     }
   }
 
   GVALinkage GetGVALinkageForFunction(const FunctionDecl *FD) const;
   GVALinkage GetGVALinkageForVariable(const VarDecl *VD);
 
+  /// Determines if the decl is the key function for a class.
+  ///
+  /// \ returns true if the function is the key and it was inline requiring
+  /// emission even if it is not used.
+  bool isKeyFunction(const CXXMethodDecl *MD);
+
   /// Determines if the decl can be CodeGen'ed or deserialized from PCH
   /// lazily, only when used; this is only relevant for function or file scoped
   /// var definitions.
   ///
   /// \returns true if the function/var must be CodeGen'ed/deserialized even if
   /// it is not used.
   bool DeclMustBeEmitted(const Decl *D);
 

lib/AST/ASTContext.cpp

 }
 
 GVALinkage ASTContext::GetGVALinkageForVariable(const VarDecl *VD) {
   return adjustGVALinkageForExternalDefinitionKind(*this, VD,
            adjustGVALinkageForAttributes(*this, VD,
              basicGVALinkageForVariable(*this, VD)));
 }
 
+bool ASTContext::isKeyFunction(const CXXMethodDecl *MD) {
+  assert(MD);
+
+  if (getTargetInfo().getCXXABI().canKeyFunctionBeInline()) {
+    const CXXRecordDecl *RD = MD->getParent();
+    if (MD->isOutOfLine() && RD->isDynamicClass()) {
+      const CXXMethodDecl *KeyFunc = getCurrentKeyFunction(RD);
+      if (KeyFunc && KeyFunc->getCanonicalDecl() == MD->getCanonicalDecl())
+        return true;
+    }
+  }
+  return false;
+}
+
 bool ASTContext::DeclMustBeEmitted(const Decl *D) {
   if (const auto *VD = dyn_cast<VarDecl>(D)) {
     if (!VD->isFileVarDecl())
       return false;
     // Global named register variables (GNU extension) are never emitted.
     if (VD->getStorageClass() == SC_Register)
       return false;
     if (VD->getDescribedVarTemplate() ||
if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     // Forward declarations aren't required.
     if (!FD->doesThisDeclarationHaveABody())
       return FD->doesDeclarationForceExternallyVisibleDefinition();
 
     // Constructors and destructors are required.
     if (FD->hasAttr<ConstructorAttr>() || FD->hasAttr<DestructorAttr>())
       return true;
 
-    // The key function for a class is required.  This rule only comes
+    // The key function for a class is required. This rule only comes
     // into play when inline functions can be key functions, though.
-    if (getTargetInfo().getCXXABI().canKeyFunctionBeInline()) {
-      if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
-        const CXXRecordDecl *RD = MD->getParent();
-        if (MD->isOutOfLine() && RD->isDynamicClass()) {
-          const CXXMethodDecl *KeyFunc = getCurrentKeyFunction(RD);
-          if (KeyFunc && KeyFunc->getCanonicalDecl() == MD->getCanonicalDecl())
-            return true;
-        }
-      }
-    }
+    if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD))
+      if (isKeyFunction(MD))
 
     GVALinkage Linkage = GetGVALinkageForFunction(FD);
 
     // static, static inline, always_inline, and extern inline functions can
     // always be deferred.  Normal inline functions can be deferred in C99/C++.
     // Implicit template instantiations can also be deferred in C++.
     return !isDiscardableGVALinkage(Linkage);
   }

lib/Serialization/ASTWriterDecl.cpp

if (isa<FileScopeAsmDecl>(D) || isa<ObjCImplDecl>(D))
     return true;
 
   if (WritingModule && (isa<VarDecl>(D) || isa<ImportDecl>(D))) {
     // These declarations are part of the module initializer, and are emitted
     // if and when the module is imported, rather than being emitted eagerly.
     return false;
   }
 
+  // If the ABI supports inline key functions we must emit them eagerly. The
+  // generic reason is that CodeGen might assume they are emitted and generate a
+  // reference to the vtable. In deserialization this rule becomes a little less
+  // clear because if a reference is generated, CodeGen will make a request and
+  // we will emit the vtable on demand.
+  //
+  // This change in behavior is driven mostly by performance considerations and
+  // is equivalent in outlining the key function (aka pinning the vtable). It
+  // also works in cases where we cannot modify the source code.
+  if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D))
+    if (Context.isKeyFunction(MD))
+      return false;
+
   return Context.DeclMustBeEmitted(D);
 }
 
 void ASTWriter::WriteDecl(ASTContext &Context, Decl *D) {
   PrettyDeclStackTraceEntry CrashInfo(Context, D, SourceLocation(),
                                       "serializing");
 
   // Determine the ID for this declaration.