Object: Shrink the size of irsymtab::Symbol by a word. NFCI.

[pcc]

Summary

Instead of storing an UncommonIndex on the Symbol, use a flag bit to store
whether the Symbol has an Uncommon. This shrinks Chromium's .bc files (after
D32061) by about 1%.

[tejohnson]

llvm/include/llvm/Object/IRSymtab.h

288 ↗

(On Diff #95263)

Should this be Uncommons.end() instead of nullptr? Otherwise, will UncI ever be nullptr allowing the SymbolRef::operator== to detect the end of the symbol_range? I see where we increment UncI, but not how it could be nullptr at the end of the Uncommons range.

llvm/lib/Object/IRSymtab.cpp

37 ↗

(On Diff #95263)

Is the restructuring in this file related to the change to set the new flag? I couldn't figure out how. It looks like a good change, but if unrelated can you commit separately?

[pcc]

llvm/include/llvm/Object/IRSymtab.h

288 ↗

(On Diff #95263)

We only use SymI for iterator comparisons, so it doesn't matter what value UncI has at the end of the range.

llvm/lib/Object/IRSymtab.cpp

37 ↗

(On Diff #95263)

I made this change to simplify setting a correct value for UncBegin in the storage::Module, which would be more difficult if we enumerated modules separately from symbols.

[tejohnson]

LGTM with one suggestion below.

llvm/include/llvm/Object/IRSymtab.h

288 ↗

(On Diff #95263)

Ok, missed that. Still, it seems odd not just to use Uncommons.end() here, instead of the less-meaningful nullptr. Suggest changing if that doesn't hurt anything.

llvm/lib/Object/IRSymtab.cpp

37 ↗

(On Diff #95263)

Got it - because Uncommons wouldn't be updated after each call to addModule.

[pcc]

llvm/include/llvm/Object/IRSymtab.h

288 ↗

(On Diff #95263)

I think that would be a little confusing. If I were reading this code and I saw Uncommons.end() here I would draw the conclusion that the pointer value in the end iterator is probably being used somehow, which would lead me to believe that there may be a bug in module_symbols() because it is passing nullptr. Using nullptr in both places makes it clear that the value is unimportant.

[tejohnson]

llvm/include/llvm/Object/IRSymtab.h

288 ↗

(On Diff #95263)

I guess it isn't straightforward to get the end iterator in the module_symbols case. Fair enough.

llvm/trunk/include/llvm/Object/IRSymtab.h

ArrayRef<T> get(StringRef Symtab) const {
     return {reinterpret_cast<const T *>(Symtab.data() + Offset), Size};
   }
 };
 
 /// Describes the range of a particular module's symbols within the symbol
 /// table.
 struct Module {
   Word Begin, End;
+
+  /// The index of the first Uncommon for this Module.
+  Word UncBegin;
 };
 
 /// This is equivalent to an IR comdat.
 struct Comdat {
   Str Name;
 };
 
 /// Contains the information needed by linkers for symbol resolution, as well as
 /// by the LTO implementation itself.
 struct Symbol {
   /// The mangled symbol name.
   Str Name;
 
   /// The unmangled symbol name, or the empty string if this is not an IR
   /// symbol.
   Str IRName;
 
   /// The index into Header::Comdats, or -1 if not a comdat member.
   Word ComdatIndex;
 
   Word Flags;
   enum FlagBits {
     FB_visibility, // 2 bits
-    FB_undefined = FB_visibility + 2,
+    FB_has_uncommon = FB_visibility + 2,
+    FB_undefined,
     FB_weak,
     FB_common,
     FB_indirect,
     FB_used,
     FB_tls,
     FB_may_omit,
     FB_global,
     FB_format_specific,
     FB_unnamed_addr,
     FB_executable,
   };
-
-  /// The index into the Uncommon table, or -1 if this symbol does not have an
-  /// Uncommon.
-  Word UncommonIndex;
 };
 
 /// This data structure contains rarely used symbol fields and is optionally
 /// referenced by a Symbol.
 struct Uncommon {
   Word CommonSize, CommonAlign;
 
   /// COFF-specific: the name of the symbol that a weak external resolves to
public:
   /// COFF-specific: returns linker options specified in the input file.
   StringRef getCOFFLinkerOpts() const { return str(header().COFFLinkerOpts); }
 };
 
 /// Ephemeral symbols produced by Reader::symbols() and
 /// Reader::module_symbols().
 class Reader::SymbolRef : public Symbol {
   const storage::Symbol *SymI, *SymE;
+  const storage::Uncommon *UncI;
   const Reader *R;
 
-public:
-  SymbolRef(const storage::Symbol *SymI, const storage::Symbol *SymE,
-            const Reader *R)
-      : SymI(SymI), SymE(SymE), R(R) {
-    read();
-  }
-
   void read() {
     if (SymI == SymE)
       return;
 
     Name = R->str(SymI->Name);
     IRName = R->str(SymI->IRName);
     ComdatIndex = SymI->ComdatIndex;
     Flags = SymI->Flags;
 
-    uint32_t UncI = SymI->UncommonIndex;
-    if (UncI != -1u) {
-      const storage::Uncommon &Unc = R->Uncommons[UncI];
-      CommonSize = Unc.CommonSize;
-      CommonAlign = Unc.CommonAlign;
-      COFFWeakExternFallbackName = R->str(Unc.COFFWeakExternFallbackName);
+    if (Flags & (1 << storage::Symbol::FB_has_uncommon)) {
+      CommonSize = UncI->CommonSize;
+      CommonAlign = UncI->CommonAlign;
+      COFFWeakExternFallbackName = R->str(UncI->COFFWeakExternFallbackName);
     }
   }
+
+public:
+  SymbolRef(const storage::Symbol *SymI, const storage::Symbol *SymE,
+            const storage::Uncommon *UncI, const Reader *R)
+      : SymI(SymI), SymE(SymE), UncI(UncI), R(R) {
+    read();
+  }
+
   void moveNext() {
     ++SymI;
+    if (Flags & (1 << storage::Symbol::FB_has_uncommon))
+      ++UncI;
     read();
   }
 
   bool operator==(const SymbolRef &Other) const { return SymI == Other.SymI; }
 };
 
 inline Reader::symbol_range Reader::symbols() const {
-  return {SymbolRef(Symbols.begin(), Symbols.end(), this),
-          SymbolRef(Symbols.end(), Symbols.end(), this)};
+  return {SymbolRef(Symbols.begin(), Symbols.end(), Uncommons.begin(), this),
+          SymbolRef(Symbols.end(), Symbols.end(), nullptr, this)};
 }
 
 inline Reader::symbol_range Reader::module_symbols(unsigned I) const {
   const storage::Module &M = Modules[I];
   const storage::Symbol *MBegin = Symbols.begin() + M.Begin,
                         *MEnd = Symbols.begin() + M.End;
-  return {SymbolRef(MBegin, MEnd, this), SymbolRef(MEnd, MEnd, this)};
+  return {SymbolRef(MBegin, MEnd, Uncommons.begin() + M.UncBegin, this),
+          SymbolRef(MEnd, MEnd, nullptr, this)};
 }
 
 }
 
 }
 
 #endif

llvm/trunk/lib/Object/IRSymtab.cpp

Builder(SmallVector<char, 0> &Symtab, SmallVector<char, 0> &Strtab)
       : Symtab(Symtab), Strtab(Strtab) {}
 
   StringTableBuilder StrtabBuilder{StringTableBuilder::RAW};
 
   BumpPtrAllocator Alloc;
   StringSaver Saver{Alloc};
 
   DenseMap<const Comdat *, unsigned> ComdatMap;
-  ModuleSymbolTable Msymtab;
-  SmallPtrSet<GlobalValue *, 8> Used;
   Mangler Mang;
   Triple TT;
 
   std::vector<storage::Comdat> Comdats;
   std::vector<storage::Module> Mods;
   std::vector<storage::Symbol> Syms;
   std::vector<storage::Uncommon> Uncommons;
 
   std::string COFFLinkerOpts;
   raw_string_ostream COFFLinkerOptsOS{COFFLinkerOpts};
 
   void setStr(storage::Str &S, StringRef Value) {
     S.Offset = StrtabBuilder.add(Value);
     S.Size = Value.size();
   }
   template <typename T>
   void writeRange(storage::Range<T> &R, const std::vector<T> &Objs) {
     R.Offset = Symtab.size();
     R.Size = Objs.size();
     Symtab.insert(Symtab.end(), reinterpret_cast<const char *>(Objs.data()),
                   reinterpret_cast<const char *>(Objs.data() + Objs.size()));
   }
 
   Error addModule(Module *M);
-  Error addSymbol(ModuleSymbolTable::Symbol Sym);
+  Error addSymbol(const ModuleSymbolTable &Msymtab,
+                  const SmallPtrSet<GlobalValue *, 8> &Used,
+                  ModuleSymbolTable::Symbol Sym);
 
   Error build(ArrayRef<Module *> Mods);
 };
 
 Error Builder::addModule(Module *M) {
+  SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(*M, Used, /*CompilerUsed*/ false);
 
-  storage::Module Mod;
-  Mod.Begin = Msymtab.symbols().size();
+  ModuleSymbolTable Msymtab;
   Msymtab.addModule(M);
-  Mod.End = Msymtab.symbols().size();
+
+  storage::Module Mod;
+  Mod.Begin = Syms.size();
+  Mod.End = Syms.size() + Msymtab.symbols().size();
+  Mod.UncBegin = Uncommons.size();
   Mods.push_back(Mod);
 
   if (TT.isOSBinFormatCOFF()) {
     if (auto E = M->materializeMetadata())
       return E;
     if (Metadata *Val = M->getModuleFlag("Linker Options")) {
       MDNode *LinkerOptions = cast<MDNode>(Val);
       for (const MDOperand &MDOptions : LinkerOptions->operands())
         for (const MDOperand &MDOption : cast<MDNode>(MDOptions)->operands())
           COFFLinkerOptsOS << " " << cast<MDString>(MDOption)->getString();
     }
   }
 
+  for (ModuleSymbolTable::Symbol Msym : Msymtab.symbols())
+    if (Error Err = addSymbol(Msymtab, Used, Msym))
+      return Err;
+
   return Error::success();
 }
 
-Error Builder::addSymbol(ModuleSymbolTable::Symbol Msym) {
+Error Builder::addSymbol(const ModuleSymbolTable &Msymtab,
+                         const SmallPtrSet<GlobalValue *, 8> &Used,
+                         ModuleSymbolTable::Symbol Msym) {
   Syms.emplace_back();
   storage::Symbol &Sym = Syms.back();
   Sym = {};
 
-  Sym.UncommonIndex = -1;
   storage::Uncommon *Unc = nullptr;
   auto Uncommon = [&]() -> storage::Uncommon & {
     if (Unc)
       return *Unc;
-    Sym.UncommonIndex = Uncommons.size();
+    Sym.Flags |= 1 << storage::Symbol::FB_has_uncommon;
     Uncommons.emplace_back();
     Unc = &Uncommons.back();
     *Unc = {};
     setStr(Unc->COFFWeakExternFallbackName, "");
     return *Unc;
   };
 
   SmallString<64> Name;

 Error Builder::build(ArrayRef<Module *> IRMods) {
   storage::Header Hdr;
 
   assert(!IRMods.empty());
   setStr(Hdr.TargetTriple, IRMods[0]->getTargetTriple());
   setStr(Hdr.SourceFileName, IRMods[0]->getSourceFileName());
   TT = Triple(IRMods[0]->getTargetTriple());
 
-  // This adds the symbols for each module to Msymtab.
   for (auto *M : IRMods)
     if (Error Err = addModule(M))
       return Err;
 
-  for (ModuleSymbolTable::Symbol Msym : Msymtab.symbols())
-    if (Error Err = addSymbol(Msym))
-      return Err;
-
   COFFLinkerOptsOS.flush();
   setStr(Hdr.COFFLinkerOpts, COFFLinkerOpts);
 
   // We are about to fill in the header's range fields, so reserve space for it
   // and copy it in afterwards.
   Symtab.resize(sizeof(storage::Header));
   writeRange(Hdr.Modules, Mods);
   writeRange(Hdr.Comdats, Comdats);