[PATCH] D31364: LTO: Reduce memory consumption by creating an in-memory symbol table for InputFiles. NFCI.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

It's a lot of code but I skimmed most of it and read a few parts in more detail. A few questions/comments.



================
Comment at: llvm/include/llvm/Object/IRSymtab.h:123
+/// Fills in Symtab and Strtab with a valid symbol and string table for Mods.
+Error write(ArrayRef<Module *> Mods, SmallVector<char, 0> &Symtab,
+            SmallVector<char, 0> &Strtab);
----------------
The name "write" here seems unexpected to me, since we aren't writing to disk e.g.. The client does a "write" which involves a Writer class, followed by a Reader, when together both are needed to essentially "read" the symbols from Modules. Maybe "buildSymbolTable" or something like that. The Writer is more like a Builder.


================
Comment at: llvm/lib/LTO/LTO.cpp:506
+  auto MsymI = SymTab.symbols().begin(), MsymE = SymTab.symbols().end();
+  auto Skip = [&]() {
+    while (MsymI != MsymE) {
----------------
Before this was embedded within symbol_iterator. What is the impact of moving it here - we only want to skip these symbols in the regular LTO case?

It's a little confusing to me that we have two symbol iterations going on in the below loop, one over the InputFile::Symbols and one over the ModuleSymbolTable - why do we need two data structures of symbols now?


================
Comment at: llvm/lib/Object/IRSymtab.cpp:24
+
+struct Writer {
+  SmallVector<char, 0> &Symtab;
----------------
Needs some comments.


https://reviews.llvm.org/D31364