Materialize metadata in IRLinker before value mapping

[tejohnson]

Summary

Unless we plan to do later postpass metadata linking (ThinLTO special mode),
always invoke metadata materialization at the start of IRLinker::run().
This avoids the need for clients who use lazy metadata loading to
explicitly invoke materializeMetadata before the IRMover, which in
turn invokes IRLinker::run and needs materialized metadata for mapping.

Came up in the context of an LLD issue (D17982).

[• rafael]

I am not sure this is better than just forcing the client to be non-lazy.

Cheers,
Rafael

[tejohnson]

My hope would be that it avoids errors and makes client lives simpler since they don't need to invoke this manually. Is there a reason why we shouldn't do it automatically?

[• rafael]

It hides the fact that the metadata is actually read eagerly.
Cheers,
Rafael

[tejohnson]

In D17992#370876, @rafael wrote:

It hides the fact that the metadata is actually read eagerly.
Cheers,
Rafael

Not sure I follow - when metadata is read eagarly (ShouldLazyLoadMetadata=false), this will be a nop.

If by "actually read eagerly" you are referring to the fact that metadata parsing is still all-or-nothing, I don't see how this patch really hides or affects that.

I'm not hugely wedded to this patch, but just want to understand why it would be a bad thing, since it would seem to make bitcode reading clients simpler.

lib/Linker/IRMover.cpp

static std::string mergeTriples(const Triple &SrcTriple,
   if (SrcTriple.getVendor() == Triple::Apple)
     if (DstTriple.isOSVersionLT(SrcTriple))
       return SrcTriple.str();
 
   return DstTriple.str();
 }
 
 bool IRLinker::run() {
+  // Ensure metadata materialized before value mapping.
+  if (shouldLinkMetadata() && SrcM->getMaterializer())
+    if (SrcM->getMaterializer()->materializeMetadata())
+      return true;
+
   // Inherit the target data from the source module if the destination module
   // doesn't have one already.
   if (DstM.getDataLayout().isDefault())
     DstM.setDataLayout(SrcM->getDataLayout());
 
   if (SrcM->getDataLayout() != DstM.getDataLayout()) {
     emitWarning("Linking two modules of different data layouts: '" +
                 SrcM->getModuleIdentifier() + "' is '" +
bool IRLinker::run() {
 
   // Remap all of the named MDNodes in Src into the DstM module. We do this
   // after linking GlobalValues so that MDNodes that reference GlobalValues
   // are properly remapped.
   if (shouldLinkMetadata()) {
     // Even if just linking metadata we should link decls above in case
     // any are referenced by metadata. IRLinker::shouldLink ensures that
     // we don't actually link anything from source.
-    if (IsMetadataLinkingPostpass) {
-      // Ensure metadata materialized
-      if (SrcM->getMaterializer()->materializeMetadata())
-        return true;
+    if (IsMetadataLinkingPostpass)
       SrcM->getMaterializer()->saveMetadataList(MetadataToIDs,
                                                 /* OnlyTempMD = */ false);
-    }
 
     linkNamedMDNodes();
 
     if (IsMetadataLinkingPostpass) {
       // Handle anything left in the ValIDToTempMDMap, such as metadata nodes
       // not reached by the dbg.cu NamedMD (i.e. only reached from
       // instructions).
       // Walk the MetadataToIDs once to find the set of new (imported) MD