[PATCH] D32061: [wip] Bitcode: Write the irsymtab to disk.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Object/IRSymtab.h:122
+  /// in symbol table format, symbol enumeration order and so on.
+  Str Producer;
+
----------------
Does this mean that the symbol table will be thrown away every time the llvm revision changes? Can we avoid that by having a symbol table version number?


================
Comment at: llvm/lib/Object/IRSymtab.cpp:304
+  if (Producer != kExpectedProducerName)
+    return upgrade(std::move(BFC.Mods));
+
----------------
I guess the disadvantage of having a fully upgradable symbol table is that we can't avoid duplication between the module bitcode and the symbol table. I.e. vs reading the symbol table first, and getting visibility and other symbol info from there and using it in the Module. Although the format if the symbol table is more compressed by not keeping it as structured bitcode records, so it is a tradeoff. 

(As an aside - is the linkage type going to be added to the symbol table so that we don't need to parse IR when building the summaries in the bitcode reader?)

I guess the approach of storing the symbol table in a custom string blob was already signed off in D31364, but I'm hoping for another pair of eyes on this. Will add Rafael to the review in case Mehdi doesn't have the bandwidth right now. Especially since we chatted about moving the ThinLTO summaries into this format.


https://reviews.llvm.org/D32061