(NFC) Move summary-based opt before regular LTO.

[eugenis]

Summary

This is necessary for CFI-ICall + ThinLTO, because the regular LTO
step of CFI-ICall needs to know the results of, at least,
summary-based dead stripping.

[pcc]

Can you just move the block with the call to computeDeadSymbols like this patch does?
https://github.com/pcc/llvm-project/commit/1731b9179a7a448469e2977aaae142fab75c6bbd

[eugenis]

Why is it better than this change?

[pcc]

If you just need the list of dead symbols, then there is no need to increase the scope of anything else. I would prefer to keep things as local as possible so that the flow of information is clearer.

include/llvm/LTO/LTO.h

private:
 
   struct ThinLTOState {
     ThinLTOState(ThinBackend Backend);
 
     ThinBackend Backend;
     ModuleSummaryIndex CombinedIndex;
     MapVector<StringRef, BitcodeModule> ModuleMap;
     DenseMap<GlobalValue::GUID, StringRef> PrevailingModuleForGUID;
+
+    StringMap<std::map<GlobalValue::GUID, GlobalValueSummary *>>
+        ModuleToDefinedGVSummaries;
+    StringMap<FunctionImporter::ImportMapTy> ImportLists;
+    StringMap<FunctionImporter::ExportSetTy> ExportLists;
+    StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
   } ThinLTO;
 
   // The global resolution for a particular (mangled) symbol name. This is in
   // particular necessary to track whether each symbol can be internalized.
   // Because any input file may introduce a new cross-partition reference, we
   // cannot make any final internalization decisions until all input files have
   // been added and the client has called run(). During run() we apply
   // internalization decisions either directly to the module (for regular LTO)
private:
   Error addRegularLTO(BitcodeModule BM,
                       ArrayRef<InputFile::Symbol> Syms,
                       const SymbolResolution *&ResI,
                       const SymbolResolution *ResE);
   Error addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
                    const SymbolResolution *&ResI, const SymbolResolution *ResE);
 
   Error runRegularLTO(AddStreamFn AddStream);
+  void prepareThinLTO();
   Error runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache,
                    bool HasRegularLTO);
 
   mutable bool CalledGetMaxTasks = false;
 };
 
 /// The resolution for a symbol. The linker must provide a SymbolResolution for
 /// each global symbol based on its internal resolution of that symbol.

lib/LTO/LTO.cpp

 }
 
 Error LTO::run(AddStreamFn AddStream, NativeObjectCache Cache) {
   // 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;
   // Invoke regular LTO if there was a regular LTO module to start with.
+  prepareThinLTO();
   if (HasRegularLTO)
     if (auto E = runRegularLTO(AddStream))
       return E;
   return runThinLTO(AddStream, Cache, HasRegularLTO);
 }
 
 Error LTO::runRegularLTO(AddStreamFn AddStream) {
   // Make sure commons have the right size/alignment: we kept the largest from
return [=](Config &Conf, ModuleSummaryIndex &CombinedIndex,
              const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
              AddStreamFn AddStream, NativeObjectCache Cache) {
     return llvm::make_unique<WriteIndexesThinBackend>(
         Conf, CombinedIndex, ModuleToDefinedGVSummaries, OldPrefix, NewPrefix,
         ShouldEmitImportsFiles, LinkedObjectsFile);
   };
 }
 
-Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache,
-                      bool HasRegularLTO) {
+void LTO::prepareThinLTO() {
   if (ThinLTO.ModuleMap.empty())
-    return Error::success();
-
-  if (Conf.CombinedIndexHook && !Conf.CombinedIndexHook(ThinLTO.CombinedIndex))
-    return Error::success();
+    return;
 
   // Collect for each module the list of function it defines (GUID ->
   // Summary).
-  StringMap<std::map<GlobalValue::GUID, GlobalValueSummary *>>
-      ModuleToDefinedGVSummaries(ThinLTO.ModuleMap.size());
   ThinLTO.CombinedIndex.collectDefinedGVSummariesPerModule(
-      ModuleToDefinedGVSummaries);
+      ThinLTO.ModuleToDefinedGVSummaries);
   // Create entries for any modules that didn't have any GV summaries
   // (either they didn't have any GVs to start with, or we suppressed
   // generation of the summaries because they e.g. had inline assembly
   // uses that couldn't be promoted/renamed on export). This is so
   // InProcessThinBackend::start can still launch a backend thread, which
   // is passed the map of summaries for the module, without any special
   // handling for this case.
   for (auto &Mod : ThinLTO.ModuleMap)
-    if (!ModuleToDefinedGVSummaries.count(Mod.first))
-      ModuleToDefinedGVSummaries.try_emplace(Mod.first);
-
-  StringMap<FunctionImporter::ImportMapTy> ImportLists(
-      ThinLTO.ModuleMap.size());
-  StringMap<FunctionImporter::ExportSetTy> ExportLists(
-      ThinLTO.ModuleMap.size());
-  StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
+    if (!ThinLTO.ModuleToDefinedGVSummaries.count(Mod.first))
+      ThinLTO.ModuleToDefinedGVSummaries.try_emplace(Mod.first);
 
   if (Conf.OptLevel > 0) {
     // Compute "dead" symbols, we don't want to import/export these!
     DenseSet<GlobalValue::GUID> GUIDPreservedSymbols;
     for (auto &Res : GlobalResolutions) {
       if (Res.second.VisibleOutsideThinLTO &&
           // IRName will be defined if we have seen the prevailing copy of
           // this value. If not, no need to preserve any ThinLTO copies.
           !Res.second.IRName.empty())
         GUIDPreservedSymbols.insert(GlobalValue::getGUID(
             GlobalValue::dropLLVMManglingEscape(Res.second.IRName)));
     }
 
     auto DeadSymbols =
         computeDeadSymbols(ThinLTO.CombinedIndex, GUIDPreservedSymbols);
 
-    ComputeCrossModuleImport(ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
-                             ImportLists, ExportLists, &DeadSymbols);
+    ComputeCrossModuleImport(
+        ThinLTO.CombinedIndex, ThinLTO.ModuleToDefinedGVSummaries,
+        ThinLTO.ImportLists, ThinLTO.ExportLists, &DeadSymbols);
 
     std::set<GlobalValue::GUID> ExportedGUIDs;
     for (auto &Res : GlobalResolutions) {
       // First check if the symbol was flagged as having external references.
       if (Res.second.Partition != GlobalResolution::External)
         continue;
       // IRName will be defined if we have seen the prevailing copy of
       // this value. If not, no need to mark as exported from a ThinLTO
       // partition (and we can't get the GUID).
       if (Res.second.IRName.empty())
         continue;
       auto GUID = GlobalValue::getGUID(
           GlobalValue::dropLLVMManglingEscape(Res.second.IRName));
       // Mark exported unless index-based analysis determined it to be dead.
       if (!DeadSymbols.count(GUID))
         ExportedGUIDs.insert(GUID);
     }
 
     auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
-      const auto &ExportList = ExportLists.find(ModuleIdentifier);
-      return (ExportList != ExportLists.end() &&
+      const auto &ExportList = ThinLTO.ExportLists.find(ModuleIdentifier);
+      return (ExportList != ThinLTO.ExportLists.end() &&
               ExportList->second.count(GUID)) ||
              ExportedGUIDs.count(GUID);
     };
     thinLTOInternalizeAndPromoteInIndex(ThinLTO.CombinedIndex, isExported);
   }
 
   auto isPrevailing = [&](GlobalValue::GUID GUID,
                           const GlobalValueSummary *S) {
     return ThinLTO.PrevailingModuleForGUID[GUID] == S->modulePath();
   };
   auto recordNewLinkage = [&](StringRef ModuleIdentifier,
                               GlobalValue::GUID GUID,
                               GlobalValue::LinkageTypes NewLinkage) {
-    ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
+    ThinLTO.ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
   };
   thinLTOResolveWeakForLinkerInIndex(ThinLTO.CombinedIndex, isPrevailing,
                                      recordNewLinkage);
+}
+
+Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache,
+                      bool HasRegularLTO) {
+  if (ThinLTO.ModuleMap.empty())
+    return Error::success();
+
+  if (Conf.CombinedIndexHook && !Conf.CombinedIndexHook(ThinLTO.CombinedIndex))
+    return Error::success();
 
   std::unique_ptr<ThinBackendProc> BackendProc =
-      ThinLTO.Backend(Conf, ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
-                      AddStream, Cache);
+      ThinLTO.Backend(Conf, ThinLTO.CombinedIndex,
+                      ThinLTO.ModuleToDefinedGVSummaries, AddStream, Cache);
 
   // Task numbers start at ParallelCodeGenParallelismLevel if an LTO
   // module is present, as tasks 0 through ParallelCodeGenParallelismLevel-1
   // are reserved for parallel code generation partitions.
   unsigned Task =
       HasRegularLTO ? RegularLTO.ParallelCodeGenParallelismLevel : 0;
   for (auto &Mod : ThinLTO.ModuleMap) {
-    if (Error E = BackendProc->start(Task, Mod.second, ImportLists[Mod.first],
-                                     ExportLists[Mod.first],
-                                     ResolvedODR[Mod.first], ThinLTO.ModuleMap))
+    if (Error E = BackendProc->start(
+            Task, Mod.second, ThinLTO.ImportLists[Mod.first],
+            ThinLTO.ExportLists[Mod.first], ThinLTO.ResolvedODR[Mod.first],
+            ThinLTO.ModuleMap))
       return E;
     ++Task;
   }
 
   return BackendProc->wait();
 }
 
 Expected<std::unique_ptr<tool_output_file>>