[PATCH] D16424: [ThinLTO] Do metadata linking during batch function importing

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:308
@@ +307,3 @@
+    // The modules were created with lazy metadata loading. Materialize it
+    // now, before linking it.
+    SrcModule->materializeMetadata();
----------------
joker.eph wrote:
> joker.eph wrote:
> > tejohnson wrote:
> > > joker.eph wrote:
> > > > I'd question why did we load them with lazy metadata in the first place? If we have materialize the metadata anyway, we can as well load them up-front.
> > > I think delaying it until here should save memory - otherwise we have all metadata for all source modules in memory at once. Materializing it here just before we link, after which the SrcModule is destroyed, should mean we only have one source module worth of metadata materialized and in memory at once (other than what is linked into the dest module, which will be a subset of the metadata, even smaller once I stop linking in the other things hanging off the DICompileUnit).
> > I'm fine with leaving the call to `materializeMetadata` as it will turn to be a no-op if they were loaded up-front.
> > Can you reflect this in the comment above? Just prepending an "If" at the beginning looks good to me.
> Actually, I have another idea: why not load metadata on demand when materializing a function?
> This seems quite easy to achieve, and would lead us to a state where we would have everything we need materialized, and nothing more.
That has the same disadvantages as not lazy loading in the first place - since we will materialize each function when we decide to import it (in order to look for external calls), we will end up materializing all metadata for all source modules we're importing from and holding them in memory at the same time.


http://reviews.llvm.org/D16424