[PATCH] D53925: [modules] Defer emission of inline key functions.

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev created this revision.
v.g.vassilev added reviewers: rsmith, bruno.
Herald added a subscriber: cfe-commits.

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!


Repository:
  rC Clang

https://reviews.llvm.org/D53925

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/Serialization/ASTWriterDecl.cpp


Index: lib/Serialization/ASTWriterDecl.cpp
===================================================================
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -2264,6 +2264,19 @@
     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);
 }
 
Index: lib/AST/ASTContext.cpp
===================================================================
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -9700,6 +9700,20 @@
              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())
@@ -9781,18 +9795,11 @@
     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))
+        return true;
 
     GVALinkage Linkage = GetGVALinkageForFunction(FD);
 
Index: include/clang/AST/ASTContext.h
===================================================================
--- include/clang/AST/ASTContext.h
+++ include/clang/AST/ASTContext.h
@@ -2733,6 +2733,12 @@
   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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D53925.171885.patch
Type: text/x-patch
Size: 3498 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181031/ac7472d6/attachment-0001.bin>