[dllimport] Don't use RAV to check inlinability

[rnk]

Summary

I don't like this change, because I had to copy a fair amount of logic
from RAV into the dllimport visitor. However, the build time and size
results were still impressive, so I wanted to upload this and get a
second opinion. Here's the data:

metric	before	after	diff
time (s)	38.7	25.3	-13.4
obj (kb)	12012	9428	-2584
exe (kb)	86000	85924	-76

Do you think I should land this as is? I'm considering factoring some of
this stuff up into a "RecursiveStmtVisitor" that tries to visit "code
that executes in a particular function scope". It could be used for
availability diagnostics that search for use of particular local
variables, etc. That seems to be the main use case we have for RAVs:
given a predicate, find all referenced decls, and check some predicate.

[hans]

Yes, it's a little bit unfortunate that the code needs to get more complicated due to this. The RecursiveStmtVisitor idea that you mention sounds like it might be a good idea (but if it turns out not to be, I'm not entirely opposed to this patch either).

clang/lib/CodeGen/CodeGenModule.cpp

 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecordLayout.h"
-#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/Module.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
bool VisitStmt(const Stmt *S) {
         if (Child && this->Visit(Child))
           return true;
       return false;
     }
   };
 
   // Make sure we're not referencing non-imported vars or functions.
   struct DLLImportFunctionVisitor
-      : public RecursiveASTVisitor<DLLImportFunctionVisitor> {
-    bool SafeToInline = true;
-
-    bool shouldVisitImplicitCode() const { return true; }
+      : public ConstStmtVisitor<DLLImportFunctionVisitor, bool> {
+    bool VisitDeclStmt(const DeclStmt *E) {
+      for (const Decl *D : E->decls()) {
+        if (const auto *VD = dyn_cast<VarDecl>(D)) {
+          if (!VisitVarDecl(VD))
+            return false;
+          // Traverse variable initializers, which are common sources of
+          // function references.
+          if (!VisitStmt(VD->getInit()))
+            return false;
+        }
+      }
+      return true;
+    }
 
-    bool VisitVarDecl(VarDecl *VD) {
-      if (VD->getTLSKind()) {
-        // A thread-local variable cannot be imported.
-        SafeToInline = false;
-        return SafeToInline;
-      }
+    bool VisitVarDecl(const VarDecl *VD) {
+      if (VD->getTLSKind())
+        return false;
 
       // A variable definition might imply a destructor call.
       if (VD->isThisDeclarationADefinition())
-        SafeToInline = !HasNonDllImportDtor(VD->getType());
+        if (HasNonDllImportDtor(VD->getType()))
+          return false;
 
-      return SafeToInline;
+      return true;
     }
 
-    bool VisitCXXBindTemporaryExpr(CXXBindTemporaryExpr *E) {
+    bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *E) {
+      bool SafeToInline = true;
       if (const auto *D = E->getTemporary()->getDestructor())
         SafeToInline = D->hasAttr<DLLImportAttr>();
       return SafeToInline;
     }
 
-    bool VisitDeclRefExpr(DeclRefExpr *E) {
-      ValueDecl *VD = E->getDecl();
+    bool VisitDeclRefExpr(const DeclRefExpr *E) {
+      const ValueDecl *VD = E->getDecl();
+      bool SafeToInline = true;
       if (isa<FunctionDecl>(VD))
         SafeToInline = VD->hasAttr<DLLImportAttr>();
-      else if (VarDecl *V = dyn_cast<VarDecl>(VD))
+      else if (const VarDecl *V = dyn_cast<VarDecl>(VD))
         SafeToInline = !V->hasGlobalStorage() || V->hasAttr<DLLImportAttr>();
       return SafeToInline;
     }
 
-    bool VisitCXXConstructExpr(CXXConstructExpr *E) {
-      SafeToInline = E->getConstructor()->hasAttr<DLLImportAttr>();
-      return SafeToInline;
+    bool VisitCXXConstructExpr(const CXXConstructExpr *E) {
+      return E->getConstructor()->hasAttr<DLLImportAttr>();
     }
 
-    bool VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {
-      CXXMethodDecl *M = E->getMethodDecl();
-      if (!M) {
-        // Call through a pointer to member function. This is safe to inline.
-        SafeToInline = true;
-      } else {
+    bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *E) {
+      // If M is null, this is a call through a pointer to member function. This
+      // is safe to inline.
+      const CXXMethodDecl *M = E->getMethodDecl();
+      bool SafeToInline = true;
+      if (M)
         SafeToInline = M->hasAttr<DLLImportAttr>();
-      }
       return SafeToInline;
     }
 
-    bool VisitCXXDeleteExpr(CXXDeleteExpr *E) {
-      SafeToInline = E->getOperatorDelete()->hasAttr<DLLImportAttr>();
-      return SafeToInline;
+    bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) {
+      return E->getOperatorDelete()->hasAttr<DLLImportAttr>();
     }
 
-    bool VisitCXXNewExpr(CXXNewExpr *E) {
-      SafeToInline = E->getOperatorNew()->hasAttr<DLLImportAttr>();
-      return SafeToInline;
+    bool VisitCXXNewExpr(const CXXNewExpr *E) {
+      return E->getOperatorNew()->hasAttr<DLLImportAttr>();
+    }
+
+    bool VisitStmt(const Stmt *S) {
+      if (!S)
+        return true;
+      for (const Stmt *Child : S->children())
+        if (Child && !this->Visit(Child))
+          return false;
+      return true;
+    }
+
+    static bool isSafeToInline(const FunctionDecl *FD) {
+      DLLImportFunctionVisitor Visitor;
+      if (!Visitor.Visit(FD->getBody()))
+        return false;
+
+      // We also need to check constructor initializers.
+      if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(FD)) {
+        for (auto *I : Ctor->inits()) {
+          if (!Visitor.Visit(I->getInit()))
+            return false;
+        }
+      }
+      return true;
     }
   };
 }
 
 // isTriviallyRecursive - Check if this function calls another
 // decl that, because of the asm attribute or the other decl being a builtin,
 // ends up pointing to itself.
 bool
bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) {
   if (getFunctionLinkage(GD) != llvm::Function::AvailableExternallyLinkage)
     return true;
   const auto *F = cast<FunctionDecl>(GD.getDecl());
   if (CodeGenOpts.OptimizationLevel == 0 && !F->hasAttr<AlwaysInlineAttr>())
     return false;
 
   if (F->hasAttr<DLLImportAttr>()) {
     // Check whether it would be safe to inline this dllimport function.
-    DLLImportFunctionVisitor Visitor;
-    Visitor.TraverseFunctionDecl(const_cast<FunctionDecl*>(F));
-    if (!Visitor.SafeToInline)
+    if (!DLLImportFunctionVisitor::isSafeToInline(F))
       return false;
 
     if (const CXXDestructorDecl *Dtor = dyn_cast<CXXDestructorDecl>(F)) {
       // Implicit destructor invocations aren't captured in the AST, so the
       // check above can't see them. Check for them manually here.
       for (const Decl *Member : Dtor->getParent()->decls())
         if (isa<FieldDecl>(Member))
           if (HasNonDllImportDtor(cast<FieldDecl>(Member)->getType()))