LTO: Port the new LTO API to ModuleSymbolTable.

[pcc]

Summary

Depends on D27076

[mehdi_amini]

llvm/include/llvm/LTO/LTO.h

79

Why don't you take all the members here?
It seems strange to see the initialization and then File->Mod = ...

[mehdi_amini]

llvm/include/llvm/LTO/LTO.h

141

Can you turn SymTab and File to be reference?

146–147

Non relevant for this patch, but it'd be nice to document getName vs getIRName, I always forget when I look at this.

220–221

Looks like something to add to ModuleSymbolTable.
(I don't mean it as a blocker for this patch)

[pcc]

• Address review comments

llvm/include/llvm/LTO/LTO.h

79

We need to create the module using the LLVMContext owned by this class.

I suppose that given that we need to do that we might as well be consistent with the other members and let MBRef be default initialized as well.

141

Done for SymTab. File may be null (see addRegularLTO) so I've left it as a pointer.

146–147

It looks like getIRName is only used by the implementation, so I've removed it in r288302 and added documentation for getName.

220–221

Maybe not, given our discussion on D27073.

[mehdi_amini]

llvm/include/llvm/LTO/LTO.h

79

OK, I missed this! But indeed I feel it is less confusing now.

146–147

It seems you need to rebase this patch?

[pcc]

• Refresh

[mehdi_amini]

llvm/include/llvm/LTO/LTO.h

84

It seems there is not much that still requires us to keep Mod here, what is fundamentally the remaining issue?

[pcc]

llvm/include/llvm/LTO/LTO.h

84

The major ones are:

• We need something to own the module
• We need to be able to query the source file name
• We need it to be able to extract used bits in add*LTO

Essentially we need it to access information that will later be stored in the bitcode symbol table. But until that's ready there's no harm in accessing the module directly.

[mehdi_amini]

llvm/include/llvm/LTO/LTO.h

84

Owning the module is only useful for the two other parts right? Technically we could store the source file name directly and extract the "used" early.
The latter seems overkill to do now considering we're moving to a bitcode symbol table.

[mehdi_amini]

LGTM

[pcc]

llvm/include/llvm/LTO/LTO.h

84

An owner is also needed for the GlobalValues stored in ModuleSymbolTable.

But that's just another example of something that can be refactored with the bitcode symbol table.

llvm/include/llvm/LTO/LTO.h

 std::string getThinLTOOutputFile(const std::string &Path,
                                  const std::string &OldPrefix,
                                  const std::string &NewPrefix);
 
 class LTO;
 struct SymbolResolution;
 class ThinBackendProc;
 
-/// An input file. This is a wrapper for IRObjectFile that exposes only the
+/// An input file. This is a wrapper for ModuleSymbolTable that exposes only the
 /// information that an LTO client should need in order to do symbol resolution.
 class InputFile {
   // FIXME: Remove LTO class friendship once we have bitcode symbol tables.
   friend LTO;
   InputFile() = default;
 
   // FIXME: Remove the LLVMContext once we have bitcode symbol tables.
   LLVMContext Ctx;
-  std::unique_ptr<object::IRObjectFile> Obj;
+  ModuleSymbolTable SymTab;
+  std::unique_ptr<Module> Mod;
+  MemoryBufferRef MBRef;
+
   std::vector<StringRef> Comdats;
   DenseMap<const Comdat *, unsigned> ComdatMap;
 
 public:
   /// Create an InputFile.
   static Expected<std::unique_ptr<InputFile>> create(MemoryBufferRef Object);
 
   class symbol_iterator;
 
-  /// This is a wrapper for object::basic_symbol_iterator that exposes only the
-  /// information that an LTO client should need in order to do symbol
-  /// resolution.
+  /// This is a wrapper for ArrayRef<ModuleSymbolTable::Symbol>::iterator that
+  /// exposes only the information that an LTO client should need in order to do
+  /// symbol resolution.
   ///
   /// This object is ephemeral; it is only valid as long as an iterator obtained
   /// from symbols() refers to it.
   class Symbol {
     friend symbol_iterator;
     friend LTO;
 
-    object::basic_symbol_iterator I;
+    ArrayRef<ModuleSymbolTable::Symbol>::iterator I;
+    const ModuleSymbolTable &SymTab;
     const InputFile *File;
-    const GlobalValue *GV;
     uint32_t Flags;
     SmallString<64> Name;
 
     bool shouldSkip() {
       return !(Flags & object::BasicSymbolRef::SF_Global) ||
              (Flags & object::BasicSymbolRef::SF_FormatSpecific);
     }
 
     void skip() {
-      const object::SymbolicFile *Obj = I->getObject();
-      auto E = Obj->symbol_end();
+      ArrayRef<ModuleSymbolTable::Symbol>::iterator E = SymTab.symbols().end();
       while (I != E) {
-        Flags = I->getFlags();
+        Flags = SymTab.getSymbolFlags(*I);
         if (!shouldSkip())
           break;
         ++I;
       }
       if (I == E)
         return;
 
       Name.clear();
       {
         raw_svector_ostream OS(Name);
-        I->printName(OS);
+        SymTab.printSymbolName(OS, *I);
       }
-      GV = cast<object::IRObjectFile>(Obj)->getSymbolGV(I->getRawDataRefImpl());
     }
 
+    bool isGV() const { return I->is<GlobalValue *>(); }
+    GlobalValue *getGV() const { return I->get<GlobalValue *>(); }
+
   public:
-    Symbol(object::basic_symbol_iterator I, const InputFile *File)
-        : I(I), File(File) {
+    Symbol(ArrayRef<ModuleSymbolTable::Symbol>::iterator I,
+           const ModuleSymbolTable &SymTab, const InputFile *File)
+        : I(I), SymTab(SymTab), File(File) {
       skip();
     }
 
     /// Returns the mangled name of the global.
     StringRef getName() const { return Name; }
 
     uint32_t getFlags() const { return Flags; }
     GlobalValue::VisibilityTypes getVisibility() const {
-      if (GV)
-        return GV->getVisibility();
+      if (isGV())
+        return getGV()->getVisibility();
       return GlobalValue::DefaultVisibility;
     }
     bool canBeOmittedFromSymbolTable() const {
-      return GV && llvm::canBeOmittedFromSymbolTable(GV);
+      return isGV() && llvm::canBeOmittedFromSymbolTable(getGV());
     }
     bool isTLS() const {
       // FIXME: Expose a thread-local flag for module asm symbols.
-      return GV && GV->isThreadLocal();
+      return isGV() && getGV()->isThreadLocal();
     }
 
     // Returns the index of the comdat this symbol is in or -1 if the symbol
     // is not in a comdat.
     // FIXME: We have to return Expected<int> because aliases point to an
     // arbitrary ConstantExpr and that might not actually be a constant. That
     // means we might not be able to find what an alias is aliased to and
     // so find its comdat.
     Expected<int> getComdatIndex() const;
 
     uint64_t getCommonSize() const {
       assert(Flags & object::BasicSymbolRef::SF_Common);
-      if (!GV)
+      if (!isGV())
         return 0;
-      return GV->getParent()->getDataLayout().getTypeAllocSize(
-          GV->getType()->getElementType());
+      return getGV()->getParent()->getDataLayout().getTypeAllocSize(
+          getGV()->getType()->getElementType());
     }
     unsigned getCommonAlignment() const {
       assert(Flags & object::BasicSymbolRef::SF_Common);
-      if (!GV)
+      if (!isGV())
         return 0;
-      return GV->getAlignment();
+      return getGV()->getAlignment();
     }
   };
 
   class symbol_iterator {
     Symbol Sym;
 
   public:
-    symbol_iterator(object::basic_symbol_iterator I, const InputFile *File)
-        : Sym(I, File) {}
+    symbol_iterator(ArrayRef<ModuleSymbolTable::Symbol>::iterator I,
+                    const ModuleSymbolTable &SymTab, const InputFile *File)
+        : Sym(I, SymTab, File) {}
 
     symbol_iterator &operator++() {
       ++Sym.I;
       Sym.skip();
       return *this;
     }
 
     symbol_iterator operator++(int) {
       symbol_iterator I = *this;
       ++*this;
       return I;
     }
 
     const Symbol &operator*() const { return Sym; }
     const Symbol *operator->() const { return &Sym; }
 
     bool operator!=(const symbol_iterator &Other) const {
       return Sym.I != Other.Sym.I;
     }
   };
 
   /// A range over the symbols in this InputFile.
   iterator_range<symbol_iterator> symbols() {
-    return llvm::make_range(symbol_iterator(Obj->symbol_begin(), this),
-                            symbol_iterator(Obj->symbol_end(), this));
+    return llvm::make_range(
+        symbol_iterator(SymTab.symbols().begin(), SymTab, this),
+        symbol_iterator(SymTab.symbols().end(), SymTab, this));
   }
 
-  StringRef getSourceFileName() const {
-    return Obj->getModule().getSourceFileName();
-  }
-
-  MemoryBufferRef getMemoryBufferRef() const {
-    return Obj->getMemoryBufferRef();
-  }
+  StringRef getSourceFileName() const { return Mod->getSourceFileName(); }
+  MemoryBufferRef getMemoryBufferRef() const { return MBRef; }
 
   // Returns a table with all the comdats used by this file.
   ArrayRef<StringRef> getComdatTable() const { return Comdats; }
 };
 
 /// This class wraps an output stream for a native object. Most clients should
 /// just be able to return an instance of this base class from the stream
 /// callback, but if a client needs to perform some action after the stream is
enum : unsigned {
       /// external reference, and therefore cannot be internalized.
       External = -2u,
     };
   };
 
   // Global mapping from mangled symbol names to resolutions.
   StringMap<GlobalResolution> GlobalResolutions;
 
-  void addSymbolToGlobalRes(object::IRObjectFile *Obj,
-                            SmallPtrSet<GlobalValue *, 8> &Used,
+  void addSymbolToGlobalRes(SmallPtrSet<GlobalValue *, 8> &Used,
                             const InputFile::Symbol &Sym, SymbolResolution Res,
                             unsigned Partition);
 
   Error addRegularLTO(std::unique_ptr<InputFile> Input,
                       ArrayRef<SymbolResolution> Res);
   Error addThinLTO(std::unique_ptr<InputFile> Input,
                    ArrayRef<SymbolResolution> Res);
 

llvm/lib/LTO/LTO.cpp

void llvm::thinLTOInternalizeAndPromoteInIndex(
     function_ref<bool(StringRef, GlobalValue::GUID)> isExported) {
   for (auto &I : Index)
     thinLTOInternalizeAndPromoteGUID(I.second, I.first, isExported);
 }
 
 Expected<std::unique_ptr<InputFile>> InputFile::create(MemoryBufferRef Object) {
   std::unique_ptr<InputFile> File(new InputFile);
 
-  Expected<std::unique_ptr<object::IRObjectFile>> IRObj =
-      IRObjectFile::create(Object, File->Ctx);
-  if (!IRObj)
-    return IRObj.takeError();
-  File->Obj = std::move(*IRObj);
+  ErrorOr<MemoryBufferRef> BCOrErr =
+      IRObjectFile::findBitcodeInMemBuffer(Object);
+  if (!BCOrErr)
+    return errorCodeToError(BCOrErr.getError());
+  File->MBRef = *BCOrErr;
 
-  for (const auto &C : File->Obj->getModule().getComdatSymbolTable()) {
+  Expected<std::unique_ptr<Module>> MOrErr =
+      getLazyBitcodeModule(*BCOrErr, File->Ctx,
+                           /*ShouldLazyLoadMetadata*/ true);
+  if (!MOrErr)
+    return MOrErr.takeError();
+
+  File->Mod = std::move(*MOrErr);
+  File->SymTab.addModule(File->Mod.get());
+
+  for (const auto &C : File->Mod->getComdatSymbolTable()) {
     auto P =
         File->ComdatMap.insert(std::make_pair(&C.second, File->Comdats.size()));
     assert(P.second);
     (void)P;
     File->Comdats.push_back(C.first());
   }
 
   return std::move(File);
 }
 
 Expected<int> InputFile::Symbol::getComdatIndex() const {
-  if (!GV)
+  if (!isGV())
     return -1;
-  const GlobalObject *GO;
-  if (auto *GA = dyn_cast<GlobalAlias>(GV)) {
-    GO = GA->getBaseObject();
-    if (!GO)
-      return make_error<StringError>("Unable to determine comdat of alias!",
-                                     inconvertibleErrorCode());
-  } else {
-    GO = cast<GlobalObject>(GV);
-  }
+  const GlobalObject *GO = getGV()->getBaseObject();
   if (const Comdat *C = GO->getComdat()) {
     auto I = File->ComdatMap.find(C);
     assert(I != File->ComdatMap.end());
     return I->second;
   }
   return -1;
 }
 

 
 LTO::LTO(Config Conf, ThinBackend Backend,
          unsigned ParallelCodeGenParallelismLevel)
     : Conf(std::move(Conf)),
       RegularLTO(ParallelCodeGenParallelismLevel, this->Conf),
       ThinLTO(std::move(Backend)) {}
 
 // Add the given symbol to the GlobalResolutions map, and resolve its partition.
-void LTO::addSymbolToGlobalRes(IRObjectFile *Obj,
-                               SmallPtrSet<GlobalValue *, 8> &Used,
+void LTO::addSymbolToGlobalRes(SmallPtrSet<GlobalValue *, 8> &Used,
                                const InputFile::Symbol &Sym,
                                SymbolResolution Res, unsigned Partition) {
-  GlobalValue *GV = Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
+  GlobalValue *GV = Sym.isGV() ? Sym.getGV() : nullptr;
 
   auto &GlobalRes = GlobalResolutions[Sym.getName()];
   if (GV) {
     GlobalRes.UnnamedAddr &= GV->hasGlobalUnnamedAddr();
     if (Res.Prevailing)
       GlobalRes.IRName = GV->getName();
   }
   if (Res.VisibleToRegularObj || (GV && Used.count(GV)) ||

 Error LTO::add(std::unique_ptr<InputFile> Input,
                ArrayRef<SymbolResolution> Res) {
   assert(!CalledGetMaxTasks);
 
   if (Conf.ResolutionFile)
     writeToResolutionFile(*Conf.ResolutionFile, Input.get(), Res);
 
   // FIXME: move to backend
-  Module &M = Input->Obj->getModule();
+  Module &M = *Input->Mod;
   if (!Conf.OverrideTriple.empty())
     M.setTargetTriple(Conf.OverrideTriple);
   else if (M.getTargetTriple().empty())
     M.setTargetTriple(Conf.DefaultTriple);
 
-  MemoryBufferRef MBRef = Input->Obj->getMemoryBufferRef();
-  Expected<bool> HasThinLTOSummary = hasGlobalValueSummary(MBRef);
+  Expected<bool> HasThinLTOSummary = hasGlobalValueSummary(Input->MBRef);
   if (!HasThinLTOSummary)
     return HasThinLTOSummary.takeError();
 
   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) {
   if (!RegularLTO.CombinedModule) {
     RegularLTO.CombinedModule =
         llvm::make_unique<Module>("ld-temp.o", RegularLTO.Ctx);
     RegularLTO.Mover = llvm::make_unique<IRMover>(*RegularLTO.CombinedModule);
   }
-  Expected<std::unique_ptr<object::IRObjectFile>> ObjOrErr =
-      IRObjectFile::create(Input->Obj->getMemoryBufferRef(), RegularLTO.Ctx);
-  if (!ObjOrErr)
-    return ObjOrErr.takeError();
-  std::unique_ptr<object::IRObjectFile> Obj = std::move(*ObjOrErr);
+  Expected<std::unique_ptr<Module>> MOrErr =
+      getLazyBitcodeModule(Input->MBRef, RegularLTO.Ctx,
+                           /*ShouldLazyLoadMetadata*/ true);
+  if (!MOrErr)
+    return MOrErr.takeError();
 
-  Module &M = Obj->getModule();
+  Module &M = **MOrErr;
   if (Error Err = M.materializeMetadata())
     return Err;
   UpgradeDebugInfo(M);
 
+  ModuleSymbolTable SymTab;
+  SymTab.addModule(&M);
+
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
 
   std::vector<GlobalValue *> Keep;
 
   for (GlobalVariable &GV : M.globals())
     if (GV.hasAppendingLinkage())
       Keep.push_back(&GV);
 
   auto ResI = Res.begin();
   for (const InputFile::Symbol &Sym :
-       make_range(InputFile::symbol_iterator(Obj->symbol_begin(), nullptr),
-                  InputFile::symbol_iterator(Obj->symbol_end(), nullptr))) {
+       make_range(InputFile::symbol_iterator(SymTab.symbols().begin(), SymTab,
+                                             nullptr),
+                  InputFile::symbol_iterator(SymTab.symbols().end(), SymTab,
+                                             nullptr))) {
     assert(ResI != Res.end());
     SymbolResolution Res = *ResI++;
-    addSymbolToGlobalRes(Obj.get(), Used, Sym, Res, 0);
+    addSymbolToGlobalRes(Used, Sym, Res, 0);
 
-    GlobalValue *GV = Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
     if (Sym.getFlags() & object::BasicSymbolRef::SF_Undefined)
       continue;
-    if (Res.Prevailing && GV) {
+    if (Res.Prevailing && Sym.isGV()) {
+      GlobalValue *GV = Sym.getGV();
       Keep.push_back(GV);
       switch (GV->getLinkage()) {
       default:
         break;
       case GlobalValue::LinkOnceAnyLinkage:
         GV->setLinkage(GlobalValue::WeakAnyLinkage);
         break;
       case GlobalValue::LinkOnceODRLinkage:
         GV->setLinkage(GlobalValue::WeakODRLinkage);
         break;
       }
     }
     // Common resolution: collect the maximum size/alignment over all commons.
     // We also record if we see an instance of a common as prevailing, so that
     // if none is prevailing we can ignore it later.
     if (Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
       // FIXME: We should figure out what to do about commons defined by asm.
       // For now they aren't reported correctly by ModuleSymbolTable.
-      assert(GV);
-      auto &CommonRes = RegularLTO.Commons[GV->getName()];
+      auto &CommonRes = RegularLTO.Commons[Sym.getGV()->getName()];
       CommonRes.Size = std::max(CommonRes.Size, Sym.getCommonSize());
       CommonRes.Align = std::max(CommonRes.Align, Sym.getCommonAlignment());
       CommonRes.Prevailing |= Res.Prevailing;
     }
 
     // FIXME: use proposed local attribute for FinalDefinitionInLinkageUnit.
   }
   assert(ResI == Res.end());
 
-  return RegularLTO.Mover->move(Obj->takeModule(), Keep,
+  return RegularLTO.Mover->move(std::move(*MOrErr), Keep,
                                 [](GlobalValue &, IRMover::ValueAdder) {},
                                 /* LinkModuleInlineAsm */ true);
 }
 
 // Add a ThinLTO object to the link.
 Error LTO::addThinLTO(std::unique_ptr<InputFile> Input,
                       ArrayRef<SymbolResolution> Res) {
-  Module &M = Input->Obj->getModule();
+  Module &M = *Input->Mod;
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
 
-  MemoryBufferRef MBRef = Input->Obj->getMemoryBufferRef();
+  MemoryBufferRef MBRef = Input->MBRef;
   Expected<std::unique_ptr<object::ModuleSummaryIndexObjectFile>>
       SummaryObjOrErr = object::ModuleSummaryIndexObjectFile::create(MBRef);
   if (!SummaryObjOrErr)
     return SummaryObjOrErr.takeError();
   ThinLTO.CombinedIndex.mergeFrom((*SummaryObjOrErr)->takeIndex(),
                                   ThinLTO.ModuleMap.size());
 
   auto ResI = Res.begin();
   for (const InputFile::Symbol &Sym : Input->symbols()) {
     assert(ResI != Res.end());
     SymbolResolution Res = *ResI++;
-    addSymbolToGlobalRes(Input->Obj.get(), Used, Sym, Res,
-                         ThinLTO.ModuleMap.size() + 1);
+    addSymbolToGlobalRes(Used, Sym, Res, ThinLTO.ModuleMap.size() + 1);
 
-    GlobalValue *GV = Input->Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
-    if (Res.Prevailing && GV)
-      ThinLTO.PrevailingModuleForGUID[GV->getGUID()] =
+    if (Res.Prevailing && Sym.isGV())
+      ThinLTO.PrevailingModuleForGUID[Sym.getGV()->getGUID()] =
           MBRef.getBufferIdentifier();
   }
   assert(ResI == Res.end());
 
   ThinLTO.ModuleMap[MBRef.getBufferIdentifier()] = MBRef;
   return Error::success();
 }