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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Object/IRSymtab.cpp:304
+  if (Producer != kExpectedProducerName)
+    return upgrade(std::move(BFC.Mods));
+
----------------
tejohnson wrote:
> pcc wrote:
> > 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.
> > 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.
> 
> Not just that - we need to know the linkage type during the thin link to do weak resolution.
You are quite right, I forgot about that. I will take a closer look at that as part of the summary overhaul to see if it can be refactored away.


https://reviews.llvm.org/D32061