[ThinLTO] Support for specifying function index from pass manager

[tejohnson]

Summary

Add a field on the PassManagerBuilder that clang or gold can use to pass
down the function index file to use for importing when the ThinLTO
backend is triggered. Add support to supply this to the function import
pass.

Also add support to the Linker::LinkModules interface used by clang
for passing down the index so that exported local values can be
promoted and renamed early.

[mehdi_amini]

include/llvm/Transforms/IPO.h

88 ↗

(On Diff #41250)

Document.

include/llvm/Transforms/IPO/PassManagerBuilder.h

120 ↗

(On Diff #41250)

Could we pass and store here directly the FunctionIndex in memory instead of a path to the file?
I can imagine some use cases where we have it in memory and want to construct the pass pipeline (ld64 plugin for instance), and the interface shouldn't force to dump to disk.

lib/Transforms/IPO/FunctionImport.cpp

243 ↗

(On Diff #41250)

Storing a string ref is error prone: the client needs to keep it alive. I'd rather store a std::string (and have all the prototype createFunctionImportPass(std::string))

258 ↗

(On Diff #41250)

This test+error is redundant with the one 3 lines below I think.

292 ↗

(On Diff #41250)

It would be nice to keep the ability to create the pass without the IndexFile, so overload instead of replace here?

[tejohnson]

Thanks for the review, responses below.

include/llvm/Transforms/IPO/PassManagerBuilder.h

120 ↗

(On Diff #41250)

Yes and this is related to a TODO I added to the companion clang patch D15025. Since we have to create a function index there (or from the gold-plugin or whatever linker is being used in the single machine version), I think we should stash the FunctionIndex on the dest Module. In that case we could use the existing index off the module if it is non-null, or the -summary-file if not. I will make that change.

lib/Transforms/IPO/FunctionImport.cpp

243 ↗

(On Diff #41250)

Ok agreed. But will be moot with the above change to stash index on module.

258 ↗

(On Diff #41250)

No, this is testing for when the user has supplied an index twice (via front-end and via -summary-file). The below error is for when the user has supplied neither. I.e. together they ensure the index must be specified exactly once.

292 ↗

(On Diff #41250)

Or just make the parameter have a default of "" as in the FunctionImportPass constructor change I made. But this will be moot with my change to stash the index on the module.

[tejohnson]

• Address Mehdi's comments.

[mehdi_amini]

include/llvm/IR/Module.h

177 ↗

(On Diff #41540)

It does not make sense to me conceptually: the FunctionIndex is not owned by a Module. It is a common data-structure for all Modules that we're dealing with.

lib/Transforms/IPO/FunctionImport.cpp

254 ↗

(On Diff #41540)

This does not seem right to me: I don't understand how you don't do a double-free when deleting the Index.

[tejohnson]

include/llvm/IR/Module.h

177 ↗

(On Diff #41540)

Ok, I had been thinking that this would be owned by the module we are importing into. But agree after thinking about the case you mentioned where there is one copy in memory in the linker, shared by all the backend threads.

Probably I should just pass the in-memory index directly in PassManagerBuilder. But the question is how to set this from clang (in D15025). The CodeGenOptions where I save the index file name doesn't seem like the right place, but perhaps I can save it on the CompilerInvocation object, and set it on the PassManagerBuilder from there. WDYT?

lib/Transforms/IPO/FunctionImport.cpp

254 ↗

(On Diff #41540)

You're right, this should just be a pointer not a unique_ptr.

[tejohnson]

include/llvm/IR/Module.h

177 ↗

(On Diff #41540)

In the new patch I will upload it is passed on the PassManagerBuilder. The FE doesn't need to stash it anywhere it turns out, it is straightforward to pass it down to where I needed it.

[tejohnson]

• Address more comments.

[tejohnson]

• Add new renameModuleForThinLTO helper, remove now unneeded LinkModules

[tejohnson]

In D15024#302839, @tejohnson wrote:

• Add new renameModuleForThinLTO helper, remove now unneeded LinkModules

arc truncated this - it should have said "remove now unneeded LinkModules change" (LinkModules is still there and needed in other contexts).

[tejohnson]

In D15024#302839, @tejohnson wrote:

• Add new renameModuleForThinLTO helper, remove now unneeded LinkModules

arc truncated this - it should have said "remove now unneeded LinkModules change" (LinkModules is still there and needed in other contexts).

[mehdi_amini]

The renameModuleForThinLTO change seems to be separate from the rest of the patch? Commit separately.

lib/Transforms/IPO/FunctionImport.cpp

242 ↗

(On Diff #41925)

Nit: /// *Optional* Function summary

[mehdi_amini]

The function index is owned by the PassManagerBuilder in this patch, which is not good: there is not guarantee it will outlive the PassManager. The PassManagerBuild should take a pointer and not own the FunctionIndex I think.

[tejohnson]

• Move ownership of FunctionInfoIndex out of PassManagerBuilder.

[mehdi_amini]

LGTM, but commit the renameModuleForThinLTO() change separately.

llvm/trunk/include/llvm/LinkAllPasses.h

ForcePassLinking() {
       (void) llvm::createDependenceAnalysisPass();
       (void) llvm::createDivergenceAnalysisPass();
       (void) llvm::createDomOnlyPrinterPass();
       (void) llvm::createDomPrinterPass();
       (void) llvm::createDomOnlyViewerPass();
       (void) llvm::createDomViewerPass();
       (void) llvm::createGCOVProfilerPass();
       (void) llvm::createInstrProfilingPass();
+      (void) llvm::createFunctionImportPass();
       (void) llvm::createFunctionInliningPass();
       (void) llvm::createAlwaysInlinerPass();
       (void) llvm::createGlobalDCEPass();
       (void) llvm::createGlobalOptimizerPass();
       (void) llvm::createGlobalsAAWrapperPass();
       (void) llvm::createIPConstantPropagationPass();
       (void) llvm::createIPSCCPPass();
       (void) llvm::createInductiveRangeCheckEliminationPass();

llvm/trunk/include/llvm/Transforms/IPO.h

 #ifndef LLVM_TRANSFORMS_IPO_H
 #define LLVM_TRANSFORMS_IPO_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace llvm {
 
+class FunctionInfoIndex;
 class ModulePass;
 class Pass;
 class Function;
 class BasicBlock;
 class GlobalValue;
 
 //===----------------------------------------------------------------------===//
 //

 /// createGVExtractionPass - If deleteFn is true, this pass deletes
 /// the specified global values. Otherwise, it deletes as much of the module as
 /// possible, except for the global values specified.
 ///
 ModulePass *createGVExtractionPass(std::vector<GlobalValue*>& GVs, bool
                                    deleteFn = false);
 
 //===----------------------------------------------------------------------===//
+/// This pass performs iterative function importing from other modules.
+ModulePass *createFunctionImportPass(FunctionInfoIndex *Index = nullptr);
+
+//===----------------------------------------------------------------------===//
 /// createFunctionInliningPass - Return a new pass object that uses a heuristic
 /// to inline direct function calls to small functions.
 ///
 /// The Threshold can be passed directly, or asked to be computed from the
 /// given optimization and size optimization arguments.
 ///
 /// The -inline-threshold command line option takes precedence over the
 /// threshold given here.

llvm/trunk/include/llvm/Transforms/IPO/PassManagerBuilder.h

 // This file defines the PassManagerBuilder class, which is used to set up a
 // "standard" optimization sequence suitable for languages like C and C++.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_IPO_PASSMANAGERBUILDER_H
 #define LLVM_TRANSFORMS_IPO_PASSMANAGERBUILDER_H
 
+#include <memory>
 #include <vector>
 
 namespace llvm {
+class FunctionInfoIndex;
 class Pass;
 class TargetLibraryInfoImpl;
 class TargetMachine;
 
 // The old pass manager infrastructure is hidden in a legacy namespace now.
 namespace legacy {
 class FunctionPassManager;
 class PassManagerBase;
public:
   /// optimizer.  If this is non-null, it is added to both the function and
   /// per-module pass pipeline.
   TargetLibraryInfoImpl *LibraryInfo;
 
   /// Inliner - Specifies the inliner to use.  If this is non-null, it is
   /// added to the per-module passes.
   Pass *Inliner;
 
+  /// The function summary index to use for function importing.
+  FunctionInfoIndex *FunctionIndex;
+
   bool DisableTailCalls;
   bool DisableUnitAtATime;
   bool DisableUnrollLoops;
   bool BBVectorize;
   bool SLPVectorize;
   bool LoopVectorize;
   bool RerollLoops;
   bool LoadCombine;

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

if (std::error_code EC = ObjOrErr.getError()) {
     Error = EC.message();
     return nullptr;
   }
   return (*ObjOrErr)->takeIndex();
 }
 
 /// Pass that performs cross-module function import provided a summary file.
 class FunctionImportPass : public ModulePass {
+  /// Optional function summary index to use for importing, otherwise
+  /// the summary-file option must be specified.
+  FunctionInfoIndex *Index;
 
 public:
   /// Pass identification, replacement for typeid
   static char ID;
 
-  explicit FunctionImportPass() : ModulePass(ID) {}
+  /// Specify pass name for debug output
+  const char *getPassName() const override {
+    return "Function Importing";
+  }
+
+  explicit FunctionImportPass(FunctionInfoIndex *Index = nullptr)
+      : ModulePass(ID), Index(Index) {}
 
   bool runOnModule(Module &M) override {
-    if (SummaryFile.empty()) {
-      report_fatal_error("error: -function-import requires -summary-file\n");
-    }
+    if (SummaryFile.empty() && !Index)
+      report_fatal_error("error: -function-import requires -summary-file or "
+                         "file from frontend\n");
+    std::unique_ptr<FunctionInfoIndex> IndexPtr;
+    if (!SummaryFile.empty()) {
+      if (Index)
+        report_fatal_error("error: -summary-file and index from frontend\n");
       std::string Error;
-    std::unique_ptr<FunctionInfoIndex> Index =
-        getFunctionIndexForFile(SummaryFile, Error, diagnosticHandler);
-    if (!Index) {
-      errs() << "Error loading file '" << SummaryFile << "': " << Error << "\n";
+      IndexPtr = getFunctionIndexForFile(SummaryFile, Error, diagnosticHandler);
+      if (!IndexPtr) {
+        errs() << "Error loading file '" << SummaryFile << "': " << Error
+               << "\n";
         return false;
       }
+      Index = IndexPtr.get();
+    }
 
     // Perform the import now.
     ModuleLazyLoaderCache Loader(M.getContext());
     FunctionImporter Importer(*Index, diagnosticHandler,
                               [&](StringRef Name)
                                   -> Module &{ return Loader(Name); });
     return Importer.importFunctions(M);
 
     return false;
   }
 };
 
 char FunctionImportPass::ID = 0;
 INITIALIZE_PASS_BEGIN(FunctionImportPass, "function-import",
                       "Summary Based Function Import", false, false)
 INITIALIZE_PASS_END(FunctionImportPass, "function-import",
                     "Summary Based Function Import", false, false)
 
 namespace llvm {
-Pass *createFunctionImportPass() { return new FunctionImportPass(); }
+Pass *createFunctionImportPass(FunctionInfoIndex *Index = nullptr) {
+  return new FunctionImportPass(Index);
+}
 }

llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp

 //===----------------------------------------------------------------------===//
 
 
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include "llvm-c/Transforms/PassManagerBuilder.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Analysis/Passes.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/FunctionInfo.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Analysis/BasicAliasAnalysis.h"
 #include "llvm/Analysis/CFLAliasAnalysis.h"
 #include "llvm/Analysis/GlobalsModRef.h"
 #include "llvm/Analysis/ScopedNoAliasAA.h"
static cl::opt<bool> EnableLoopLoadElim(
     "enable-loop-load-elim", cl::init(false), cl::Hidden,
     cl::desc("Enable the new, experimental LoopLoadElimination Pass"));
 
 PassManagerBuilder::PassManagerBuilder() {
     OptLevel = 2;
     SizeLevel = 0;
     LibraryInfo = nullptr;
     Inliner = nullptr;
+    FunctionIndex = nullptr;
     DisableUnitAtATime = false;
     DisableUnrollLoops = false;
     BBVectorize = RunBBVectorization;
     SLPVectorize = RunSLPVectorization;
     LoopVectorize = RunLoopVectorization;
     RerollLoops = RunLoopRerolling;
     LoadCombine = RunLoadCombine;
     DisableGVNLoadPRE = false;
void PassManagerBuilder::populateModulePassManager(
 
   addExtensionsToPM(EP_OptimizerLast, MPM);
 }
 
 void PassManagerBuilder::addLTOOptimizationPasses(legacy::PassManagerBase &PM) {
   // Provide AliasAnalysis services for optimizations.
   addInitialAliasAnalysisPasses(PM);
 
+  if (FunctionIndex)
+    PM.add(createFunctionImportPass(FunctionIndex));
+
   // Propagate constants at call sites into the functions they call.  This
   // opens opportunities for globalopt (and inlining) by substituting function
   // pointers passed as arguments to direct uses of functions.
   PM.add(createIPSCCPPass());
 
   // Now that we internalized some globals, see if we can hack on them!
   PM.add(createFunctionAttrsPass()); // Add norecurse if possible.
   PM.add(createGlobalOptimizerPass());