[LTO] NFC: remove unique_ptr from some LTO::RegularLTOState members

[vitalybuka]

Summary

LTO::RegularLTOState::CombinedModule and LTO::RegularLTOState::Mover do not have
to be pointers.

LTO::RegularLTOState::HasCombinedModule is needed to have this patch as NFC.
It should be removed by D41267.

[mehdi_amini]

This patch doesn't seem motivated, the only effect I can see is that a ThinLTO link that don't include any LTO module would create a module while it was lazily created only if needed before.

llvm/include/llvm/Transforms/Utils/SplitModule.h

39

You're changing the semantic here by not transferring ownership of the module.
This is not a const reference, which indicate side-effects on the module, but you don't document what are these.

[pcc]

llvm/include/llvm/Transforms/Utils/SplitModule.h

37

Sorry, I forgot about the fact that this function takes a unique_ptr. In that case it would probably be fine to leave the CombinedModule field as a unique_ptr but just move its initialization to the constructor.

[vitalybuka]

In D41274#956435, @mehdi_amini wrote:

This patch doesn't seem motivated, the only effect I can see is that a ThinLTO link that don't include any LTO module would create a module while it was lazily created only if needed before.

Yes, this was motivated only by comment on D41267.

llvm/include/llvm/LTO/LTO.h

struct CommonResolution {
       unsigned Align = 0;
       /// Record if at least one instance of the common was marked as prevailing
       bool Prevailing = false;
     };
     std::map<std::string, CommonResolution> Commons;
 
     unsigned ParallelCodeGenParallelismLevel;
     LTOLLVMContext Ctx;
-    bool HasModule = false;
-    std::unique_ptr<Module> CombinedModule;
-    std::unique_ptr<IRMover> Mover;
+    bool HasCombinedModule = false;
+    Module CombinedModule;
+    IRMover Mover;
 
     // This stores the information about a regular LTO module that we have added
     // to the link. It will either be linked immediately (for modules without
     // summaries) or after summary-based dead stripping (for modules with
     // summaries).
     struct AddedModule {
       std::unique_ptr<Module> M;
       std::vector<GlobalValue *> Keep;

llvm/include/llvm/LTO/LTOBackend.h

 class Module;
 class Target;
 
 namespace lto {
 
 /// Runs a regular LTO backend. The regular LTO backend can also act as the
 /// regular LTO phase of ThinLTO, which may need to access the combined index.
 Error backend(Config &C, AddStreamFn AddStream,
-              unsigned ParallelCodeGenParallelismLevel,
-              std::unique_ptr<Module> M, ModuleSummaryIndex &CombinedIndex);
+              unsigned ParallelCodeGenParallelismLevel, Module &M,
+              ModuleSummaryIndex &CombinedIndex);
 
 /// Runs a ThinLTO backend.
 Error thinBackend(Config &C, unsigned Task, AddStreamFn AddStream, Module &M,
                   const ModuleSummaryIndex &CombinedIndex,
                   const FunctionImporter::ImportMapTy &ImportList,
                   const GVSummaryMapTy &DefinedGlobals,
                   MapVector<StringRef, BitcodeModule> &ModuleMap);
 }
 }
 
 #endif

llvm/include/llvm/Transforms/Utils/SplitModule.h

 /// FIXME: This function does not deal with the somewhat subtle symbol
 /// visibility issues around module splitting, including (but not limited to):
 ///
 /// - Internal symbols should not collide with symbols defined outside the
 ///   module.
 /// - Internal symbols defined in module-level inline asm should be visible to
 ///   each partition.
 void SplitModule(
-    std::unique_ptr<Module> M, unsigned N,
+    Module &M, unsigned N,
     function_ref<void(std::unique_ptr<Module> MPart)> ModuleCallback,
     bool PreserveLocals = false);
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_UTILS_SPLITMODULE_H

llvm/lib/CodeGen/ParallelCG.cpp

std::unique_ptr<Module> llvm::splitCodeGen(
 
   // Create ThreadPool in nested scope so that threads will be joined
   // on destruction.
   {
     ThreadPool CodegenThreadPool(OSs.size());
     int ThreadCount = 0;
 
     SplitModule(
-        std::move(M), OSs.size(),
+        *M, OSs.size(),
         [&](std::unique_ptr<Module> MPart) {
           // We want to clone the module in a new context to multi-thread the
           // codegen. We do it by serializing partition modules to bitcode
           // (while still on the main thread, in order to avoid data races) and
           // spinning up new threads which deserialize the partitions into
           // separate contexts.
           // FIXME: Provide a more direct way to do this in LLVM.
           SmallString<0> BC;

llvm/lib/LTO/LTO.cpp

 
 StringRef InputFile::getName() const {
   return Mods[0].getModuleIdentifier();
 }
 
 LTO::RegularLTOState::RegularLTOState(unsigned ParallelCodeGenParallelismLevel,
                                       Config &Conf)
     : ParallelCodeGenParallelismLevel(ParallelCodeGenParallelismLevel),
-      Ctx(Conf) {}
+      Ctx(Conf), CombinedModule("ld-temp.o", Ctx), Mover(CombinedModule) {}
 
 LTO::ThinLTOState::ThinLTOState(ThinBackend Backend) : Backend(Backend) {
   if (!Backend)
     this->Backend =
         createInProcessThinBackend(llvm::heavyweight_hardware_concurrency());
 }
 
 LTO::LTO(Config Conf, ThinBackend Backend,
if (!M.getComdatSymbolTable().empty())
     for (GlobalValue &GV : M.global_values())
       handleNonPrevailingComdat(GV, NonPrevailingComdats);
   assert(MsymI == MsymE);
   return std::move(Mod);
 }
 
 Error LTO::linkRegularLTO(RegularLTOState::AddedModule Mod,
                           bool LivenessFromIndex) {
-  if (!RegularLTO.CombinedModule) {
-    RegularLTO.CombinedModule =
-        llvm::make_unique<Module>("ld-temp.o", RegularLTO.Ctx);
-    RegularLTO.Mover = llvm::make_unique<IRMover>(*RegularLTO.CombinedModule);
-  }
-
+  RegularLTO.HasCombinedModule = true;
   std::vector<GlobalValue *> Keep;
   for (GlobalValue *GV : Mod.Keep) {
     if (LivenessFromIndex && !ThinLTO.CombinedIndex.isGUIDLive(GV->getGUID()))
       continue;
 
     if (!GV->hasAvailableExternallyLinkage()) {
       Keep.push_back(GV);
       continue;
     }
 
     // Only link available_externally definitions if we don't already have a
     // definition.
     GlobalValue *CombinedGV =
-        RegularLTO.CombinedModule->getNamedValue(GV->getName());
+        RegularLTO.CombinedModule.getNamedValue(GV->getName());
     if (CombinedGV && !CombinedGV->isDeclaration())
       continue;
 
     Keep.push_back(GV);
   }
 
-  return RegularLTO.Mover->move(std::move(Mod.M), Keep,
-                                [](GlobalValue &, IRMover::ValueAdder) {},
-                                /* IsPerformingImport */ false);
+  return RegularLTO.Mover.move(std::move(Mod.M), Keep,
 }
 
 // Add a ThinLTO module to the link.
 Error LTO::addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
                       const SymbolResolution *&ResI,
                       const SymbolResolution *ResE) {
   if (Error Err =
           BM.readSummary(ThinLTO.CombinedIndex, BM.getModuleIdentifier(),
if (Res.second.VisibleOutsideSummary &&
           GlobalValue::dropLLVMManglingEscape(Res.second.IRName)));
   }
 
   computeDeadSymbols(ThinLTO.CombinedIndex, GUIDPreservedSymbols);
 
   // Save the status of having a regularLTO combined module, as
   // this is needed for generating the ThinLTO Task ID, and
   // the CombinedModule will be moved at the end of runRegularLTO.
-  bool HasRegularLTO = RegularLTO.CombinedModule != nullptr ||
-                       !RegularLTO.ModsWithSummaries.empty();
+  bool HasRegularLTO =
+      RegularLTO.HasCombinedModule || !RegularLTO.ModsWithSummaries.empty();
   // Invoke regular LTO if there was a regular LTO module to start with.
   if (HasRegularLTO)
     if (auto E = runRegularLTO(AddStream))
       return E;
   return runThinLTO(AddStream, Cache, HasRegularLTO);
 }
 
 Error LTO::runRegularLTO(AddStreamFn AddStream) {
   for (auto &M : RegularLTO.ModsWithSummaries)
     if (Error Err = linkRegularLTO(std::move(M),
                                    /*LivenessFromIndex=*/true))
       return Err;
 
   // Make sure commons have the right size/alignment: we kept the largest from
   // all the prevailing when adding the inputs, and we apply it here.
-  const DataLayout &DL = RegularLTO.CombinedModule->getDataLayout();
+  const DataLayout &DL = RegularLTO.CombinedModule.getDataLayout();
   for (auto &I : RegularLTO.Commons) {
     if (!I.second.Prevailing)
       // Don't do anything if no instance of this common was prevailing.
       continue;
-    GlobalVariable *OldGV = RegularLTO.CombinedModule->getNamedGlobal(I.first);
+    GlobalVariable *OldGV = RegularLTO.CombinedModule.getNamedGlobal(I.first);
     if (OldGV && DL.getTypeAllocSize(OldGV->getValueType()) == I.second.Size) {
       // Don't create a new global if the type is already correct, just make
       // sure the alignment is correct.
       OldGV->setAlignment(I.second.Align);
       continue;
     }
     ArrayType *Ty =
         ArrayType::get(Type::getInt8Ty(RegularLTO.Ctx), I.second.Size);
-    auto *GV = new GlobalVariable(*RegularLTO.CombinedModule, Ty, false,
+    auto *GV = new GlobalVariable(RegularLTO.CombinedModule, Ty, false,
                                   GlobalValue::CommonLinkage,
                                   ConstantAggregateZero::get(Ty), "");
     GV->setAlignment(I.second.Align);
     if (OldGV) {
       OldGV->replaceAllUsesWith(ConstantExpr::getBitCast(GV, OldGV->getType()));
       GV->takeName(OldGV);
       OldGV->eraseFromParent();
     } else {
       GV->setName(I.first);
     }
   }
 
   if (Conf.PreOptModuleHook &&
-      !Conf.PreOptModuleHook(0, *RegularLTO.CombinedModule))
+      !Conf.PreOptModuleHook(0, RegularLTO.CombinedModule))
     return Error::success();
 
   if (!Conf.CodeGenOnly) {
     for (const auto &R : GlobalResolutions) {
       if (R.second.IRName.empty())
         continue;
       if (R.second.Partition != 0 &&
           R.second.Partition != GlobalResolution::External)
         continue;
 
       GlobalValue *GV =
-          RegularLTO.CombinedModule->getNamedValue(R.second.IRName);
+          RegularLTO.CombinedModule.getNamedValue(R.second.IRName);
       // Ignore symbols defined in other partitions.
       if (!GV || GV->hasLocalLinkage())
         continue;
       GV->setUnnamedAddr(R.second.UnnamedAddr ? GlobalValue::UnnamedAddr::Global
                                               : GlobalValue::UnnamedAddr::None);
       if (R.second.Partition == 0)
         GV->setLinkage(GlobalValue::InternalLinkage);
     }
 
     if (Conf.PostInternalizeModuleHook &&
-        !Conf.PostInternalizeModuleHook(0, *RegularLTO.CombinedModule))
+        !Conf.PostInternalizeModuleHook(0, RegularLTO.CombinedModule))
       return Error::success();
   }
   return backend(Conf, AddStream, RegularLTO.ParallelCodeGenParallelismLevel,
-                 std::move(RegularLTO.CombinedModule), ThinLTO.CombinedIndex);
+                 RegularLTO.CombinedModule, ThinLTO.CombinedIndex);
 }
 
 /// This class defines the interface to the ThinLTO backend.
 class lto::ThinBackendProc {
 protected:
   Config &Conf;
   ModuleSummaryIndex &CombinedIndex;
   const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries;

llvm/lib/LTO/LTOBackend.cpp

void codegen(Config &Conf, TargetMachine *TM, AddStreamFn AddStream,
   auto Stream = AddStream(Task);
   legacy::PassManager CodeGenPasses;
   if (TM->addPassesToEmitFile(CodeGenPasses, *Stream->OS, Conf.CGFileType))
     report_fatal_error("Failed to setup codegen");
   CodeGenPasses.run(Mod);
 }
 
 void splitCodeGen(Config &C, TargetMachine *TM, AddStreamFn AddStream,
-                  unsigned ParallelCodeGenParallelismLevel,
-                  std::unique_ptr<Module> Mod) {
+                  unsigned ParallelCodeGenParallelismLevel, Module &Mod) {
   ThreadPool CodegenThreadPool(ParallelCodeGenParallelismLevel);
   unsigned ThreadCount = 0;
   const Target *T = &TM->getTarget();
 
   SplitModule(
-      std::move(Mod), ParallelCodeGenParallelismLevel,
+      Mod, ParallelCodeGenParallelismLevel,
       [&](std::unique_ptr<Module> MPart) {
         // We want to clone the module in a new context to multi-thread the
         // codegen. We do it by serializing partition modules to bitcode
         // (while still on the main thread, in order to avoid data races) and
         // spinning up new threads which deserialize the partitions into
         // separate contexts.
         // FIXME: Provide a more direct way to do this in LLVM.
         SmallString<0> BC;
finalizeOptimizationRemarks(std::unique_ptr<ToolOutputFile> DiagOutputFile) {
   // call the global destructors before exiting.
   if (!DiagOutputFile)
     return;
   DiagOutputFile->keep();
   DiagOutputFile->os().flush();
 }
 
 Error lto::backend(Config &C, AddStreamFn AddStream,
-                   unsigned ParallelCodeGenParallelismLevel,
-                   std::unique_ptr<Module> Mod,
+                   unsigned ParallelCodeGenParallelismLevel, Module &Mod,
                    ModuleSummaryIndex &CombinedIndex) {
-  Expected<const Target *> TOrErr = initAndLookupTarget(C, *Mod);
+  Expected<const Target *> TOrErr = initAndLookupTarget(C, Mod);
   if (!TOrErr)
     return TOrErr.takeError();
 
-  std::unique_ptr<TargetMachine> TM = createTargetMachine(C, *TOrErr, *Mod);
+  std::unique_ptr<TargetMachine> TM = createTargetMachine(C, *TOrErr, Mod);
 
   // Setup optimization remarks.
   auto DiagFileOrErr = lto::setupOptimizationRemarks(
-      Mod->getContext(), C.RemarksFilename, C.RemarksWithHotness);
+      Mod.getContext(), C.RemarksFilename, C.RemarksWithHotness);
   if (!DiagFileOrErr)
     return DiagFileOrErr.takeError();
   auto DiagnosticOutputFile = std::move(*DiagFileOrErr);
 
   if (!C.CodeGenOnly) {
-    if (!opt(C, TM.get(), 0, *Mod, /*IsThinLTO=*/false,
+    if (!opt(C, TM.get(), 0, Mod, /*IsThinLTO=*/false,
              /*ExportSummary=*/&CombinedIndex, /*ImportSummary=*/nullptr)) {
       finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
       return Error::success();
     }
   }
 
   if (ParallelCodeGenParallelismLevel == 1) {
-    codegen(C, TM.get(), AddStream, 0, *Mod);
+    codegen(C, TM.get(), AddStream, 0, Mod);
   } else {
-    splitCodeGen(C, TM.get(), AddStream, ParallelCodeGenParallelismLevel,
-                 std::move(Mod));
+    splitCodeGen(C, TM.get(), AddStream, ParallelCodeGenParallelismLevel, Mod);
   }
   finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
   return Error::success();
 }
 
 Error lto::thinBackend(Config &Conf, unsigned Task, AddStreamFn AddStream,
                        Module &Mod, const ModuleSummaryIndex &CombinedIndex,
                        const FunctionImporter::ImportMapTy &ImportList,

llvm/lib/Transforms/Utils/SplitModule.cpp

if (const Function *F = dyn_cast<Function>(&GV)) {
         BlockAddress *BA = BlockAddress::lookup(&BB);
         if (!BA || !BA->isConstantUsed())
           continue;
         addAllGlobalValueUsers(GVtoClusterMap, F, BA);
       }
     }
 
     if (GV.hasLocalLinkage())
-      addAllGlobalValueUsers(GVtoClusterMap, &GV, &GV);
-  };
-
-  llvm::for_each(M->functions(), recordGVSet);
-  llvm::for_each(M->globals(), recordGVSet);
-  llvm::for_each(M->aliases(), recordGVSet);
-
-  // Assigned all GVs to merged clusters while balancing number of objects in
-  // each.
+      addAllGlobalValueUsers(GVtoClusterMap, &GV, &GV);
+  };
+
+  llvm::for_each(M->functions(), recordGVSet);
+  llvm::for_each(M->globals(), recordGVSet);
+  llvm::for_each(M->aliases(), recordGVSet);
+
+  // Assigned all GVs to merged clusters while balancing number of objects in
+  // each.
   auto CompareClusters = [](const std::pair<unsigned, unsigned> &a,
                             const std::pair<unsigned, unsigned> &b) {
     if (a.second || b.second)
       return a.second > b.second;
     else
       return a.first > b.first;
   };
 
static bool isInPartition(const GlobalValue *GV, unsigned I, unsigned N) {
   MD5 H;
   MD5::MD5Result R;
   H.update(Name);
   H.final(R);
   return (R[0] | (R[1] << 8)) % N == I;
 }
 
 void llvm::SplitModule(
-    std::unique_ptr<Module> M, unsigned N,
+    Module &M, unsigned N,
     function_ref<void(std::unique_ptr<Module> MPart)> ModuleCallback,
     bool PreserveLocals) {
   if (!PreserveLocals) {
-    for (Function &F : *M)
+    for (Function &F : M)
       externalize(&F);
-    for (GlobalVariable &GV : M->globals())
+    for (GlobalVariable &GV : M.globals())
       externalize(&GV);
-    for (GlobalAlias &GA : M->aliases())
+    for (GlobalAlias &GA : M.aliases())
       externalize(&GA);
-    for (GlobalIFunc &GIF : M->ifuncs())
+    for (GlobalIFunc &GIF : M.ifuncs())
       externalize(&GIF);
   }
 
   // This performs splitting without a need for externalization, which might not
   // always be possible.
   ClusterIDMapType ClusterIDMap;
-  findPartitions(M.get(), ClusterIDMap, N);
+  findPartitions(&M, ClusterIDMap, N);
 
   // FIXME: We should be able to reuse M as the last partition instead of
   // cloning it.
   for (unsigned I = 0; I < N; ++I) {
     ValueToValueMapTy VMap;
     std::unique_ptr<Module> MPart(
-        CloneModule(M.get(), VMap, [&](const GlobalValue *GV) {
+        CloneModule(&M, VMap, [&](const GlobalValue *GV) {
           if (ClusterIDMap.count(GV))
             return (ClusterIDMap[GV] == I);
           else
             return isInPartition(GV, I, N);
         }));
     if (I != 0)
       MPart->setModuleInlineAsm("");
     ModuleCallback(std::move(MPart));
   }
 }

llvm/tools/llvm-split/llvm-split.cpp

int main(int argc, char **argv) {
   std::unique_ptr<Module> M = parseIRFile(InputFilename, Err, Context);
 
   if (!M) {
     Err.print(argv[0], errs());
     return 1;
   }
 
   unsigned I = 0;
-  SplitModule(std::move(M), NumOutputs, [&](std::unique_ptr<Module> MPart) {
+  SplitModule(*M, NumOutputs, [&](std::unique_ptr<Module> MPart) {
     std::error_code EC;
     std::unique_ptr<ToolOutputFile> Out(
         new ToolOutputFile(OutputFilename + utostr(I++), EC, sys::fs::F_None));
     if (EC) {
       errs() << EC.message() << '\n';
       exit(1);
     }