FunctionImport: add a progressive heuristic to limit importing too deep in the callgraph

The current function importer will walk the callgraph, importing
transitively any callee that is below the threshold. This can
lead to import very deep which is costly in compile time and not
necessarily beneficial as most of the inline would happen in
imported function and not necessarilly in user code.

The actual factor has been carefully chosen by flipping a coin ;)
Some tuning need to be done (just at the existing limiting threshold).

Reviewers: tejohnson

Differential Revision: http://reviews.llvm.org/D17082

From: Mehdi Amini <mehdi.amini@apple.com>
llvm-svn: 260466

llvm/lib/Transforms/IPO/FunctionImport.cpp

@@ -37,6 +37,13 @@ static cl::opt<unsigned> ImportInstrLimit(
     "import-instr-limit", cl::init(100), cl::Hidden, cl::value_desc("N"),
     cl::desc("Only import functions with less than N instructions"));
 
+static cl::opt<float>
+    ImportInstrFactor("import-instr-evolution-factor", cl::init(0.7),
+                      cl::Hidden, cl::value_desc("x"),
+                      cl::desc("As we import functions, multiply the "
+                               "`import-instr-limit` threshold by this factor "
+                               "before processing newly imported functions"));
+
 // Load lazily a module from \p FileName in \p Context.
 static std::unique_ptr<Module> loadFile(const std::string &FileName,
                                         LLVMContext &Context) {
@@ -56,6 +63,14 @@ static std::unique_ptr<Module> loadFile(const std::string &FileName,
 }
 
 namespace {
+
+/// Track functions already seen using a map that record the current
+/// Threshold and the importing decision. Since the traversal of the call graph
+/// is DFS, we can revisit a function a second time with a higher threshold. In
+/// this case and if the function was not imported the first time, it is added
+/// back to the worklist with the new threshold
+using VisitedFunctionTrackerTy = StringMap<std::pair<unsigned, bool>>;
+
 /// Helper to load on demand a Module from file and cache it for subsequent
 /// queries. It can be used with the FunctionImporter.
 class ModuleLazyLoaderCache {
@@ -93,12 +108,12 @@ Module &ModuleLazyLoaderCache::operator()(StringRef Identifier) {
 } // anonymous namespace
 
 /// Walk through the instructions in \p F looking for external
-/// calls not already in the \p CalledFunctions set. If any are
+/// calls not already in the \p VisitedFunctions map. If any are
 /// found they are added to the \p Worklist for importing.
-static void findExternalCalls(const Module &DestModule, Function &F,
-                              const FunctionInfoIndex &Index,
-                              StringSet<> &CalledFunctions,
-                              SmallVector<StringRef, 64> &Worklist) {
+static void findExternalCalls(
+    const Module &DestModule, Function &F, const FunctionInfoIndex &Index,
+    VisitedFunctionTrackerTy &VisitedFunctions, unsigned Threshold,
+    SmallVectorImpl<std::pair<StringRef, unsigned>> &Worklist) {
   // We need to suffix internal function calls imported from other modules,
   // prepare the suffix ahead of time.
   std::string Suffix;
@@ -130,10 +145,18 @@ static void findExternalCalls(const Module &DestModule, Function &F,
         auto CalledFunctionGlobalID = Function::getGlobalIdentifier(
             CalledFunction->getName(), CalledFunction->getLinkage(),
             CalledFunction->getParent()->getSourceFileName());
-        auto It = CalledFunctions.insert(CalledFunctionGlobalID);
+
+        auto CalledFunctionInfo = std::make_pair(Threshold, false);
+        auto It = VisitedFunctions.insert(
+            std::make_pair(CalledFunctionGlobalID, CalledFunctionInfo));
         if (!It.second) {
-          // This is a call to a function we already considered, skip.
+          // This is a call to a function we already considered, if the function
+          // has been imported the first time, or if the current threshold is
+          // not higher, skip it.
+          auto &FunctionInfo = It.first->second;
+          if (FunctionInfo.second || FunctionInfo.first >= Threshold)
             continue;
+          It.first->second = CalledFunctionInfo;
         }
         // Ignore functions already present in the destination module
         auto *SrcGV = DestModule.getNamedValue(ImportedName);
@@ -148,7 +171,7 @@ static void findExternalCalls(const Module &DestModule, Function &F,
           }
         }
 
-        Worklist.push_back(It.first->getKey());
+        Worklist.push_back(std::make_pair(It.first->getKey(), Threshold));
         DEBUG(dbgs() << DestModule.getModuleIdentifier()
                      << ": Adding callee for : " << ImportedName << " : "
                      << F.getName() << "\n");
@@ -165,17 +188,21 @@ static void findExternalCalls(const Module &DestModule, Function &F,
 //
 // \p ModuleToFunctionsToImportMap is filled with the set of Function to import
 // per Module.
-static void GetImportList(Module &DestModule,
-                          SmallVector<StringRef, 64> &Worklist,
-                          StringSet<> &CalledFunctions,
-                          std::map<StringRef, DenseSet<const GlobalValue *>>
-                              &ModuleToFunctionsToImportMap,
+static void
+GetImportList(Module &DestModule,
+              SmallVectorImpl<std::pair<StringRef, unsigned>> &Worklist,
+              VisitedFunctionTrackerTy &VisitedFunctions,
+              std::map<StringRef, DenseSet<const GlobalValue *>> &
+                  ModuleToFunctionsToImportMap,
               const FunctionInfoIndex &Index,
               ModuleLazyLoaderCache &ModuleLoaderCache) {
   while (!Worklist.empty()) {
-    auto CalledFunctionName = Worklist.pop_back_val();
+    StringRef CalledFunctionName;
+    unsigned Threshold;
+    std::tie(CalledFunctionName, Threshold) = Worklist.pop_back_val();
     DEBUG(dbgs() << DestModule.getModuleIdentifier() << ": Process import for "
-                 << CalledFunctionName << "\n");
+                 << CalledFunctionName << " with Threshold " << Threshold
+                 << "\n");
 
     // Try to get a summary for this function call.
     auto InfoList = Index.findFunctionInfoList(CalledFunctionName);
@@ -199,13 +226,17 @@ static void GetImportList(Module &DestModule,
       llvm_unreachable("Missing summary");
     }
 
-    if (Summary->instCount() > ImportInstrLimit) {
+    if (Summary->instCount() > Threshold) {
       DEBUG(dbgs() << DestModule.getModuleIdentifier() << ": Skip import of "
                    << CalledFunctionName << " with " << Summary->instCount()
-                   << " instructions (limit " << ImportInstrLimit << ")\n");
+                   << " instructions (limit " << Threshold << ")\n");
       continue;
     }
 
+    // Mark the function as imported in the VisitedFunctions tracker
+    assert(VisitedFunctions.count(CalledFunctionName));
+    VisitedFunctions[CalledFunctionName].second = true;
+
     // Get the module path from the summary.
     auto ModuleIdentifier = Summary->modulePath();
     DEBUG(dbgs() << DestModule.getModuleIdentifier() << ": Importing "
@@ -256,8 +287,11 @@ static void GetImportList(Module &DestModule,
     Entry.insert(F);
 
     // Process the newly imported functions and add callees to the worklist.
+    // Adjust the threshold
+    Threshold = Threshold * ImportInstrFactor;
     F->materialize();
-    findExternalCalls(DestModule, *F, Index, CalledFunctions, Worklist);
+    findExternalCalls(DestModule, *F, Index, VisitedFunctions, Threshold,
+                      Worklist);
   }
 }
 
@@ -271,13 +305,15 @@ bool FunctionImporter::importFunctions(Module &DestModule) {
                << DestModule.getModuleIdentifier() << "\n");
   unsigned ImportedCount = 0;
 
-  /// First step is collecting the called external functions.
-  StringSet<> CalledFunctions;
-  SmallVector<StringRef, 64> Worklist;
+  // First step is collecting the called external functions.
+  // We keep the function name as well as the import threshold for its callees.
+  VisitedFunctionTrackerTy VisitedFunctions;
+  SmallVector<std::pair<StringRef, unsigned>, 64> Worklist;
   for (auto &F : DestModule) {
     if (F.isDeclaration() || F.hasFnAttribute(Attribute::OptimizeNone))
       continue;
-    findExternalCalls(DestModule, F, Index, CalledFunctions, Worklist);
+    findExternalCalls(DestModule, F, Index, VisitedFunctions, ImportInstrLimit,
+                      Worklist);
   }
   if (Worklist.empty())
     return false;
@@ -294,7 +330,7 @@ bool FunctionImporter::importFunctions(Module &DestModule) {
   // Analyze the summaries and get the list of functions to import by
   // populating ModuleToFunctionsToImportMap
   ModuleLazyLoaderCache ModuleLoaderCache(ModuleLoader);
-  GetImportList(DestModule, Worklist, CalledFunctions,
+  GetImportList(DestModule, Worklist, VisitedFunctions,
                 ModuleToFunctionsToImportMap, Index, ModuleLoaderCache);
   assert(Worklist.empty() && "Worklist hasn't been flushed in GetImportList");
 

llvm/test/Transforms/FunctionImport/Inputs/adjustable_threshold.ll

@@ -0,0 +1,37 @@
+define void @globalfunc1() {
+entry:
+  call void @trampoline()
+  ret void
+}
+; Adds an artificial level in the call graph to reduce the importing threshold
+define void @trampoline() {
+entry:
+  call void @largefunction()
+  ret void
+}
+
+define void @globalfunc2() {
+entry:
+  call void @largefunction()
+  ret void
+}
+
+
+; Size is 5: if two layers below in the call graph the threshold will be 4,
+; but if only one layer below the threshold will be 7.
+define void @largefunction() {
+  entry:
+  call void @staticfunc2()
+  call void @staticfunc2()
+  call void @staticfunc2()
+  call void @staticfunc2()
+  call void @staticfunc2()
+  ret void
+}
+
+define internal void @staticfunc2() {
+entry:
+  ret void
+}
+
+

llvm/test/Transforms/FunctionImport/adjustable_threshold.ll

@@ -0,0 +1,31 @@
+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: llvm-as -function-summary %s -o %t.bc
+; RUN: llvm-as -function-summary %p/Inputs/adjustable_threshold.ll -o %t2.bc
+; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
+
+; Test import with default progressive instruction factor
+; RUN: opt -function-import -summary-file %t3.thinlto.bc %s -import-instr-limit=10 -S | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIM-DEFAULT
+; INSTLIM-DEFAULT: call void @staticfunc2.llvm.2()
+
+; Test import with a reduced progressive instruction factor
+; RUN: opt -function-import -summary-file %t3.thinlto.bc %s -import-instr-limit=10 -import-instr-evolution-factor=0.5 -S | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIM-PROGRESSIVE
+; INSTLIM-PROGRESSIVE-NOT: call void @staticfunc
+
+
+
+declare void @globalfunc1()
+declare void @globalfunc2()
+
+define void @entry() {
+entry:
+; Call site are processed in reversed order!
+
+; On the direct call, we reconsider @largefunction with a higher threshold and
+; import it
+  call void @globalfunc2()
+; When importing globalfunc1, the threshold was limited and @largefunction was
+; not imported.
+  call void @globalfunc1()
+  ret void
+}
+