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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/Object/IRSymtab.h:122
+  /// in symbol table format, symbol enumeration order and so on.
+  Str Producer;
+
----------------
tejohnson wrote:
> 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?
That would be tricky to get right. There are many things that could change the symbol table, even without changing the format.

For example, a change such as D31443 would re-order the symbol table, despite not touching the bitcode reader at all.

As another example, suppose that we decide to remove an intrinsic from LLVM, and we upgrade existing IR by removing the intrinsic declaration (and changing calls to do something else). That by itself would change the symbol table.

So I think the safest option would be to accept only symbol tables from the same revision of LLVM.


================
Comment at: llvm/lib/Object/IRSymtab.cpp:304
+  if (Producer != kExpectedProducerName)
+    return upgrade(std::move(BFC.Mods));
+
----------------
tejohnson wrote:
> 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.
> 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.

Yes, although to some extent the duplication is unavoidable because we have to be able to store module inline asm symbols here as well.

> (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 believe that the only use of the linkage when building the summary is to determine whether a global has external linkage. There is already a symbol flag `FB_global` that gives us that information.


https://reviews.llvm.org/D32061