[LTO] Stop always creating and running an LTO compilation if there is not a single LTO object

[mehdi_amini]

Summary

I assume there was a use case, so maybe this strawman patch will help
clarifying if it is legit.
In any case the current situation is not legit: a ThinLTO compilation
should not trigger an unexpected full LTO compilation.
Right now, adding a --save-temps option triggers this and makes the
number of output differs.

[tejohnson]

Thanks for working on this one! I believe this will cause an issue for gold as it is setting up a PreOptModuleHook to add common symbols to the combined module. However, I wonder if this is not working anyway, as the bug I mentioned on IRC appears to be related to a common symbol that is being internalized improperly. Let me see if I can get a better handle on what is going on there this morning. I don't recall the reasons pcc wanted these to be added to a separate module rather than marked prevailing in the current module.

Regardless, I think it is probably better for the client to indicate explicitly to the API when it needs a combined module to be constructed, rather than inferring by the existence of any hooks.

[mehdi_amini]

I understand that gold adds symbol to the first module, what I'm not sure about is why it needs to be the combined one, what is the impact of adding these to the first ThinLTO module instead?

[mehdi_amini]

Make sure ThinLTO task starts at 0 when no LTO module is present.

[tejohnson]

Giving this a try, but in the meantime there are a couple fixes needed to get the compile to succeed, noted below.

lib/LTO/LTO.cpp

307 ↗

(On Diff #68351)

CombinedModule, Mover and Ctx need to all be prefixed with "RegularLTO." in this block of code.

357 ↗

(On Diff #68351)

Mover is now a pointer.

[mehdi_amini]

Shame on me for submitting patches that don't build...

[tejohnson]

lib/LTO/LTO.cpp

710 ↗

(On Diff #68371)

This change causes a different failure. We assert in thinLTOInternalizeModule/MustPreserveGV for the first ThinLTO module. The internalizer calls back to MustPreserveGV for the added common, but we assert because it is not in the DefinedGlobals since there was no summary for it in that module.

However, I found that with my internalize fix for commons we don't need this change with your patch. We simply keep the common symbol in the module where it was originally defined, as a common, which is what happened before the new API. Let me do some more testing with that fix, and see if it fixes the rest of the cpu2006 benchmark failures I was getting. If that works, then you could submit this patch (minus this particular change) afterwards.

However, while testing things I encountered another issue where the new API is going to cause problems for the distributed build system case. I first reverted this particular part of your patch and tested it on 403.gcc without my internalization fix. I expected to get undefs for the common symbols, thinking that they wouldn't be defined anywhere (we internalize the original copy and remove it since it didn't have uses there, and we would presumably not add it to any combined module which we shouldn't have with ThinLTO). However, we did end up with a combined module - there are several modules that don't have summary indexes, I believe because they contain inline assembly and we are currently preventing issues there by suppressing index generation). The test in LTO::add causes them to be added to a single combined module. This will be a problem for a distributed build where we compile with the thinlto-index-only plugin option as we will not get any index files for these files. The distributed build system is going to barf because it wants those as input to backend actions for those modules. I already hit a related issue since with the new API we stopped generating these files for the modules the linker decided not to include in the link, which is a change from the earlier behavior where gold would always emit the individual indexes (and imports files if requested) for all modules unconditionally, and I was planning to make a change to address this. I will also change things so we add modules as ThinLTO unconditionally under an option passed from the client.

[mehdi_amini]

What the current status for this one, with all the recent patches that got in?

[tejohnson]

In D23600#522256, @mehdi_amini wrote:

What the current status for this one, with all the recent patches that got in?

The commons fix is in, so this one should be good to go with the change noted below. Why don't I test it right now with SPEC just to be sure...

lib/LTO/LTO.cpp

710 ↗

(On Diff #68371)

However, I found that with my internalize fix for commons we don't need this change with your patch.
We simply keep the common symbol in the module where it was originally defined, as a common, which is
what happened before the new API. Let me do some more testing with that fix, and see if it fixes the rest of
the cpu2006 benchmark failures I was getting. If that works, then you could submit this patch (minus this
particular change) afterwards.

Since the commons fix is in, this change can be reverted.

[mehdi_amini]

lib/LTO/LTO.cpp

710 ↗

(On Diff #68371)

Why wouldn't we want this change? I would include this independently from the commons thing

[tejohnson]

lib/LTO/LTO.cpp

710 ↗

(On Diff #68371)

I suppose with the commons fix this won't cause the failure I described with symbols being added unexpectedly to a ThinLTO module...so maybe it is ok now. The comment above needs to be fixed though. I am building a compiler from this which I will test with SPEC...

[tejohnson]

Passes SPEC

llvm/trunk/include/llvm/LTO/LTO.h

struct CommonResolution {
       unsigned Align = 0;
     };
     std::map<std::string, CommonResolution> Commons;
 
     unsigned ParallelCodeGenParallelismLevel;
     LTOLLVMContext Ctx;
     bool HasModule = false;
     std::unique_ptr<Module> CombinedModule;
-    IRMover Mover;
+    std::unique_ptr<IRMover> Mover;
   } RegularLTO;
 
   struct ThinLTOState {
     ThinLTOState(ThinBackend Backend);
 
     ThinBackend Backend;
     ModuleSummaryIndex CombinedIndex;
     MapVector<StringRef, MemoryBufferRef> ModuleMap;

llvm/trunk/lib/LTO/LTO.cpp

Expected<std::unique_ptr<InputFile>> InputFile::create(MemoryBufferRef Object) {
   File->Ctx.setDiagnosticHandler(nullptr, nullptr);
 
   return std::move(File);
 }
 
 LTO::RegularLTOState::RegularLTOState(unsigned ParallelCodeGenParallelismLevel,
                                       Config &Conf)
     : ParallelCodeGenParallelismLevel(ParallelCodeGenParallelismLevel),
-      Ctx(Conf), CombinedModule(llvm::make_unique<Module>("ld-temp.o", Ctx)),
-      Mover(*CombinedModule) {}
+      Ctx(Conf) {}
 
 LTO::ThinLTOState::ThinLTOState(ThinBackend Backend) : Backend(Backend) {
   if (!Backend)
     this->Backend = createInProcessThinBackend(thread::hardware_concurrency());
 }
 
 LTO::LTO(Config Conf, ThinBackend Backend,
          unsigned ParallelCodeGenParallelismLevel)
if (HasThinLTOSummary)
     return addThinLTO(std::move(Input), Res);
   else
     return addRegularLTO(std::move(Input), Res);
 }
 
 // Add a regular LTO object to the link.
 Error LTO::addRegularLTO(std::unique_ptr<InputFile> Input,
                          ArrayRef<SymbolResolution> Res) {
-  RegularLTO.HasModule = true;
-
+  if (!RegularLTO.CombinedModule) {
+    RegularLTO.CombinedModule =
+        llvm::make_unique<Module>("ld-temp.o", RegularLTO.Ctx);
+    RegularLTO.Mover = llvm::make_unique<IRMover>(*RegularLTO.CombinedModule);
+  }
   ErrorOr<std::unique_ptr<object::IRObjectFile>> ObjOrErr =
       IRObjectFile::create(Input->Obj->getMemoryBufferRef(), RegularLTO.Ctx);
   if (!ObjOrErr)
     return errorCodeToError(ObjOrErr.getError());
   std::unique_ptr<object::IRObjectFile> Obj = std::move(*ObjOrErr);
 
   Module &M = Obj->getModule();
   M.materializeMetadata();
if (Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
       CommonRes.Size = std::max(CommonRes.Size, Sym.getCommonSize());
       CommonRes.Align = std::max(CommonRes.Align, Sym.getCommonAlignment());
     }
 
     // FIXME: use proposed local attribute for FinalDefinitionInLinkageUnit.
   }
   assert(ResI == Res.end());
 
-  return RegularLTO.Mover.move(Obj->takeModule(), Keep,
+  return RegularLTO.Mover->move(Obj->takeModule(), Keep,
                                 [](GlobalValue &, IRMover::ValueAdder) {});
 }
 
 // Add a ThinLTO object to the link.
 Error LTO::addThinLTO(std::unique_ptr<InputFile> Input,
                       ArrayRef<SymbolResolution> Res) {
   Module &M = Input->Obj->getModule();
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
 
-  // We need to initialize the target info for the combined regular LTO module
-  // in case we have no regular LTO objects. In that case we still need to build
-  // it as usual because the client may want to add symbol definitions to it.
-  if (RegularLTO.CombinedModule->getTargetTriple().empty()) {
-    RegularLTO.CombinedModule->setTargetTriple(M.getTargetTriple());
-    RegularLTO.CombinedModule->setDataLayout(M.getDataLayout());
-  }
-
   MemoryBufferRef MBRef = Input->Obj->getMemoryBufferRef();
   ErrorOr<std::unique_ptr<object::ModuleSummaryIndexObjectFile>>
       SummaryObjOrErr =
           object::ModuleSummaryIndexObjectFile::create(MBRef, Conf.DiagHandler);
   if (!SummaryObjOrErr)
     return errorCodeToError(SummaryObjOrErr.getError());
   ThinLTO.CombinedIndex.mergeFrom((*SummaryObjOrErr)->takeIndex(),
                                   ThinLTO.ModuleMap.size());

 }
 
 unsigned LTO::getMaxTasks() const {
   CalledGetMaxTasks = true;
   return RegularLTO.ParallelCodeGenParallelismLevel + ThinLTO.ModuleMap.size();
 }
 
 Error LTO::run(AddOutputFn AddOutput) {
-  // Invoke regular LTO if there was a regular LTO module to start with,
-  // or if there are any hooks that the linker may have used to add
-  // its own resolved symbols to the combined module.
-  if (RegularLTO.HasModule || Conf.PreOptModuleHook ||
-      Conf.PostInternalizeModuleHook || Conf.PostOptModuleHook ||
-      Conf.PreCodeGenModuleHook)
+  // Invoke regular LTO if there was a regular LTO module to start with.
+  if (RegularLTO.CombinedModule)
     if (auto E = runRegularLTO(AddOutput))
       return E;
   return runThinLTO(AddOutput);
 }
 
 Error LTO::runRegularLTO(AddOutputFn AddOutput) {
   // 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.
Error LTO::runThinLTO(AddOutputFn AddOutput) {
   std::unique_ptr<ThinBackendProc> BackendProc = ThinLTO.Backend(
       Conf, ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries, AddOutput);
 
   // Partition numbers for ThinLTO jobs start at 1 (see comments for
   // GlobalResolution in LTO.h). Task numbers, however, start at
   // ParallelCodeGenParallelismLevel, as tasks 0 through
   // ParallelCodeGenParallelismLevel-1 are reserved for parallel code generation
   // partitions.
-  unsigned Task = RegularLTO.ParallelCodeGenParallelismLevel;
+  unsigned Task = RegularLTO.CombinedModule
+                      ? RegularLTO.ParallelCodeGenParallelismLevel
+                      : 0;
   unsigned Partition = 1;
 
   for (auto &Mod : ThinLTO.ModuleMap) {
     if (Error E = BackendProc->start(Task, Mod.second, ImportLists[Mod.first],
                                      ThinLTO.ModuleMap))
       return E;
 
     ++Task;
     ++Partition;
   }
 
   return BackendProc->wait();
 }