[PM] Rework the new PM support for building the ModuleSummaryIndex to directly produce the index as the value type result.

[chandlerc]

Summary

This requires making the index movable which is straightforward. It
greatly simplifies things by allowing us to completely avoid the builder
API and the layers of abstraction inherent there. Instead both pass
managers can directly construct these when run by value. They still
won't be constructed truly eagerly thanks to the optional in the legacy
PM. The code that directly builds the index can also just share a direct
function.

A notable change here is that the result type of the analysis for the
new PM is no longer a reference type. This was really problematic when
making changes to how we handle result types to make our interface
requirements *much* more strict and precise. But I think this is an
overall improvement.

[mehdi_amini]

LGTM.

I like functions better than classes when possible :)

include/llvm/Analysis/ModuleSummaryAnalysis.h

45

You define a typedef for Result two lines above, why not using it here?

I don't think it is used anywhere, if it is only used by the pass manager infrastructure, maybe add a comment above the typedef.

[chandlerc]

include/llvm/Analysis/ModuleSummaryAnalysis.h

45

Happy to use the typedef if you prefer.

But the typedef is for the pass manager infrastructure. I can comment it, but it is a fairly pervasive pattern so not sure how much value that comment adds. Thoughts? Would you like a comment on all the Result typedefs?

[mehdi_amini]

include/llvm/Analysis/ModuleSummaryAnalysis.h

45

Well you're right, since it is new we're not used to see it everywhere, but since it is gonna be everywhere we probably won't need to replicate the one line comment on each and every instance.

[tejohnson]

LGTM. Thanks for the cleanup!

include/llvm/Analysis/ModuleSummaryAnalysis.h

 #define LLVM_ANALYSIS_MODULESUMMARYANALYSIS_H
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
 
 namespace llvm {
-
 class BlockFrequencyInfo;
 
-/// Class to build a module summary index for the given Module, possibly from
-/// a Pass.
-class ModuleSummaryIndexBuilder {
-  /// The index being built
-  std::unique_ptr<ModuleSummaryIndex> Index;
-  /// The module for which we are building an index
-  const Module *M;
-
-public:
-  /// Default constructor
-  ModuleSummaryIndexBuilder() = default;
-
-  /// Constructor that builds an index for the given Module. An optional
-  /// callback can be supplied to obtain the frequency info for a function.
-  ModuleSummaryIndexBuilder(
-      const Module *M,
-      std::function<BlockFrequencyInfo *(const Function &F)> Ftor = nullptr);
-
-  /// Get a reference to the index owned by builder
-  ModuleSummaryIndex &getIndex() const { return *Index; }
-
-  /// Take ownership of the built index
-  std::unique_ptr<ModuleSummaryIndex> takeIndex() { return std::move(Index); }
-
-private:
-  /// Compute summary for given function with optional frequency information
-  void computeFunctionSummary(const Function &F,
-                              BlockFrequencyInfo *BFI = nullptr);
-
-  /// Compute summary for given variable with optional frequency information
-  void computeVariableSummary(const GlobalVariable &V);
-};
+/// Direct function to compute a \c ModuleSummaryIndex from a given module.
+///
+/// If operating within a pass manager which has defined ways to compute the \c
+/// BlockFrequencyInfo for a given function, that can be provided via
+/// a std::function callback. Otherwise, this routine will manually construct
+/// that information.
+ModuleSummaryIndex buildModuleSummaryIndex(
+    const Module &M,
+    std::function<BlockFrequencyInfo *(const Function &F)> GetBFICallback =
+        nullptr);
 
 /// Analysis pass to provide the ModuleSummaryIndex object.
 class ModuleSummaryIndexAnalysis
     : public AnalysisInfoMixin<ModuleSummaryIndexAnalysis> {
   friend AnalysisInfoMixin<ModuleSummaryIndexAnalysis>;
   static char PassID;
 
-  std::unique_ptr<ModuleSummaryIndexBuilder> IndexBuilder;
-
 public:
-  typedef const ModuleSummaryIndex &Result;
+  typedef ModuleSummaryIndex Result;
 
-  // FIXME: Remove these once MSVC can synthesize them.
-  ModuleSummaryIndexAnalysis() {}
-  ModuleSummaryIndexAnalysis(ModuleSummaryIndexAnalysis &&Arg)
-      : IndexBuilder(std::move(Arg.IndexBuilder)) {}
-  ModuleSummaryIndexAnalysis &operator=(ModuleSummaryIndexAnalysis &&RHS) {
-    IndexBuilder = std::move(RHS.IndexBuilder);
-    return *this;
-  }
-
-  const ModuleSummaryIndex &run(Module &M, ModuleAnalysisManager &AM);
+  ModuleSummaryIndex run(Module &M, ModuleAnalysisManager &AM);
 };
 
 /// Legacy wrapper pass to provide the ModuleSummaryIndex object.
 class ModuleSummaryIndexWrapperPass : public ModulePass {
-  std::unique_ptr<ModuleSummaryIndexBuilder> IndexBuilder;
+  Optional<ModuleSummaryIndex> Index;
 
 public:
   static char ID;
 
   ModuleSummaryIndexWrapperPass();
 
   /// Get the index built by pass
-  ModuleSummaryIndex &getIndex() { return IndexBuilder->getIndex(); }
-  const ModuleSummaryIndex &getIndex() const {
-    return IndexBuilder->getIndex();
-  }
+  ModuleSummaryIndex &getIndex() { return *Index; }
+  const ModuleSummaryIndex &getIndex() const { return *Index; }
 
   bool runOnModule(Module &M) override;
   bool doFinalization(Module &M) override;
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 };
 
 //===--------------------------------------------------------------------===//
 //

include/llvm/IR/ModuleSummaryIndex.h

private:
   /// name (may be duplicates in the COMDAT case, e.g.).
   GlobalValueSummaryMapTy GlobalValueMap;
 
   /// Holds strings for combined index, mapping to the corresponding module ID.
   ModulePathStringTableTy ModulePathStringTable;
 
 public:
   ModuleSummaryIndex() = default;
-
-  // Disable the copy constructor and assignment operators, so
-  // no unexpected copying/moving occurs.
-  ModuleSummaryIndex(const ModuleSummaryIndex &) = delete;
-  void operator=(const ModuleSummaryIndex &) = delete;
+  ModuleSummaryIndex(ModuleSummaryIndex &&Arg)
+      : GlobalValueMap(std::move(Arg.GlobalValueMap)),
+        ModulePathStringTable(std::move(Arg.ModulePathStringTable)) {}
+  ModuleSummaryIndex &operator=(ModuleSummaryIndex &&RHS) {
+    GlobalValueMap = std::move(RHS.GlobalValueMap);
+    ModulePathStringTable = std::move(RHS.ModulePathStringTable);
+    return *this;
+  }
 
   gvsummary_iterator begin() { return GlobalValueMap.begin(); }
   const_gvsummary_iterator begin() const { return GlobalValueMap.begin(); }
   gvsummary_iterator end() { return GlobalValueMap.end(); }
   const_gvsummary_iterator end() const { return GlobalValueMap.end(); }
 
   /// Get the list of global value summary objects for a given value name.
   const GlobalValueSummaryList &getGlobalValueSummaryList(StringRef ValueName) {

lib/Analysis/ModuleSummaryAnalysis.cpp

for (const auto &OI : U->operands()) {
           RefEdges.insert(Operand);
         continue;
       }
       Worklist.push_back(Operand);
     }
   }
 }
 
-void ModuleSummaryIndexBuilder::computeFunctionSummary(
+static void computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
                                    const Function &F, BlockFrequencyInfo *BFI) {
   // Summary not currently supported for anonymous functions, they must
   // be renamed.
   if (!F.hasName())
     return;
 
   unsigned NumInsts = 0;
   // Map from callee ValueId to profile count. Used to accumulate profile
   // counts for all static calls to a given callee.
for (const Instruction &I : BB) {
 
       if (auto CS = ImmutableCallSite(&I)) {
         auto *CalledFunction = CS.getCalledFunction();
         // Check if this is a direct call to a known function.
         if (CalledFunction) {
           if (CalledFunction->hasName() && !CalledFunction->isIntrinsic()) {
             auto ScaledCount = BFI ? BFI->getBlockProfileCount(&BB) : None;
             auto *CalleeId =
-                M->getValueSymbolTable().lookup(CalledFunction->getName());
+                M.getValueSymbolTable().lookup(CalledFunction->getName());
             CallGraphEdges[CalleeId] +=
                 (ScaledCount ? ScaledCount.getValue() : 0);
           }
         } else {
           // Otherwise, check for an indirect call (call to a non-const value
           // that isn't an inline assembly call).
           const CallInst *CI = dyn_cast<CallInst>(&I);
           if (CS.getCalledValue() && !isa<Constant>(CS.getCalledValue()) &&
for (const BasicBlock &BB : F)
     }
 
   GlobalValueSummary::GVFlags Flags(F);
   std::unique_ptr<FunctionSummary> FuncSummary =
       llvm::make_unique<FunctionSummary>(Flags, NumInsts);
   FuncSummary->addCallGraphEdges(CallGraphEdges);
   FuncSummary->addCallGraphEdges(IndirectCallEdges);
   FuncSummary->addRefEdges(RefEdges);
-  Index->addGlobalValueSummary(F.getName(), std::move(FuncSummary));
+  Index.addGlobalValueSummary(F.getName(), std::move(FuncSummary));
 }
 
-void ModuleSummaryIndexBuilder::computeVariableSummary(
+static void computeVariableSummary(ModuleSummaryIndex &Index,
                                    const GlobalVariable &V) {
   DenseSet<const Value *> RefEdges;
   SmallPtrSet<const User *, 8> Visited;
   findRefEdges(&V, RefEdges, Visited);
   GlobalValueSummary::GVFlags Flags(V);
   std::unique_ptr<GlobalVarSummary> GVarSummary =
       llvm::make_unique<GlobalVarSummary>(Flags);
   GVarSummary->addRefEdges(RefEdges);
-  Index->addGlobalValueSummary(V.getName(), std::move(GVarSummary));
+  Index.addGlobalValueSummary(V.getName(), std::move(GVarSummary));
 }
 
-ModuleSummaryIndexBuilder::ModuleSummaryIndexBuilder(
-    const Module *M,
-    std::function<BlockFrequencyInfo *(const Function &F)> Ftor)
-    : Index(llvm::make_unique<ModuleSummaryIndex>()), M(M) {
+ModuleSummaryIndex llvm::buildModuleSummaryIndex(
+    const Module &M,
+    std::function<BlockFrequencyInfo *(const Function &F)> GetBFICallback) {
+  ModuleSummaryIndex Index;
   // Check if the module can be promoted, otherwise just disable importing from
   // it by not emitting any summary.
   // FIXME: we could still import *into* it most of the time.
-  if (!moduleCanBeRenamedForThinLTO(*M))
-    return;
+  if (!moduleCanBeRenamedForThinLTO(M))
+    return Index;
 
   // Compute summaries for all functions defined in module, and save in the
   // index.
-  for (auto &F : *M) {
+  for (auto &F : M) {
     if (F.isDeclaration())
       continue;
 
     BlockFrequencyInfo *BFI = nullptr;
     std::unique_ptr<BlockFrequencyInfo> BFIPtr;
-    if (Ftor)
-      BFI = Ftor(F);
+    if (GetBFICallback)
+      BFI = GetBFICallback(F);
     else if (F.getEntryCount().hasValue()) {
       LoopInfo LI{DominatorTree(const_cast<Function &>(F))};
       BranchProbabilityInfo BPI{F, LI};
       BFIPtr = llvm::make_unique<BlockFrequencyInfo>(F, BPI, LI);
       BFI = BFIPtr.get();
     }
 
-    computeFunctionSummary(F, BFI);
+    computeFunctionSummary(Index, M, F, BFI);
   }
 
   // Compute summaries for all variables defined in module, and save in the
   // index.
-  for (const GlobalVariable &G : M->globals()) {
+  for (const GlobalVariable &G : M.globals()) {
     if (G.isDeclaration())
       continue;
-    computeVariableSummary(G);
+    computeVariableSummary(Index, G);
   }
+  return Index;
 }
 
 char ModuleSummaryIndexAnalysis::PassID;
 
-const ModuleSummaryIndex &
+ModuleSummaryIndex
 ModuleSummaryIndexAnalysis::run(Module &M, ModuleAnalysisManager &AM) {
   auto &FAM = AM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
-  IndexBuilder = llvm::make_unique<ModuleSummaryIndexBuilder>(
-      &M, [&FAM](const Function &F) {
-        return &(
-            FAM.getResult<BlockFrequencyAnalysis>(*const_cast<Function *>(&F)));
-      });
-  return IndexBuilder->getIndex();
+  return buildModuleSummaryIndex(M, [&FAM](const Function &F) {
+    return &FAM.getResult<BlockFrequencyAnalysis>(*const_cast<Function *>(&F));
 }
 
 char ModuleSummaryIndexWrapperPass::ID = 0;
 INITIALIZE_PASS_BEGIN(ModuleSummaryIndexWrapperPass, "module-summary-analysis",
                       "Module Summary Analysis", false, true)
 INITIALIZE_PASS_DEPENDENCY(BlockFrequencyInfoWrapperPass)
 INITIALIZE_PASS_END(ModuleSummaryIndexWrapperPass, "module-summary-analysis",
                     "Module Summary Analysis", false, true)
 
 ModulePass *llvm::createModuleSummaryIndexWrapperPass() {
   return new ModuleSummaryIndexWrapperPass();
 }
 
 ModuleSummaryIndexWrapperPass::ModuleSummaryIndexWrapperPass()
     : ModulePass(ID) {
   initializeModuleSummaryIndexWrapperPassPass(*PassRegistry::getPassRegistry());
 }
 
 bool ModuleSummaryIndexWrapperPass::runOnModule(Module &M) {
-  IndexBuilder = llvm::make_unique<ModuleSummaryIndexBuilder>(
-      &M, [this](const Function &F) {
-        return &(this->getAnalysis<BlockFrequencyInfoWrapperPass>(
-                         *const_cast<Function *>(&F))
-                     .getBFI());
-      });
+  Index = buildModuleSummaryIndex(M, [this](const Function &F) {
   return false;
 }
 
 bool ModuleSummaryIndexWrapperPass::doFinalization(Module &M) {
-  IndexBuilder.reset();
+  Index.reset();
   return false;
 }
 
 void ModuleSummaryIndexWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.setPreservesAll();
   AU.addRequired<BlockFrequencyInfoWrapperPass>();
 }
 

lib/LTO/ThinLTOCodeGenerator.cpp

ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
 
   saveTempBitcode(TheModule, SaveTempsDir, count, ".4.opt.bc");
 
   if (DisableCodeGen) {
     // Configured to stop before CodeGen, serialize the bitcode and return.
     SmallVector<char, 128> OutputBuffer;
     {
       raw_svector_ostream OS(OutputBuffer);
-      ModuleSummaryIndexBuilder IndexBuilder(&TheModule);
-      WriteBitcodeToFile(&TheModule, OS, true, &IndexBuilder.getIndex());
+      auto Index = buildModuleSummaryIndex(TheModule);
+      WriteBitcodeToFile(&TheModule, OS, true, &Index);
     }
     return make_unique<ObjectMemoryBuffer>(std::move(OutputBuffer));
   }
 
   return codegenModule(TheModule, TM);
 }
 
 /// Resolve LinkOnce/Weak symbols. Record resolutions in the \p ResolvedODR map