[lto] Don't lazy load metadata for now.

[silvas]

Summary

At the very least we hit

Assertion failed: (((Flags & RF_HaveUnmaterializedMetadata) || Node->isResolved()) && "Unexpected unresolved node"), function MapMetadataImpl, file /Users/Sean/pg/llvm/lib/Transforms/Utils/ValueMapper.cpp, line 375.

on the included test case.

We currently do things like parse the module twice to keep the
implementation minimal. I think it makes sense to add start with eager
loading for similar reasons.

[tejohnson]

With lazy metadata loading something needs to call materializeMetadata before the IRMover is invoked. Not sure there is an advantage to delaying it in this case, but see the suggestion below for where that could be done.

Rafael, should the IRLinker just always invoke materializeMetadata on the module (if shouldLinkMetadata() is true, which will avoid doing that if we plan to do post-pass metadata linking for ThinLTO)? I'm already invoking materializeMetadata when we detect we are in the postpass md linking. Probably could just move that outside the IsMetadataLinkingPostpass guard in IRMover.cpp. I can send a patch right now to do that, I don't know how to test lld but Sean can give it a try.

ELF/SymbolTable.cpp

147

Call M->materializeMetadata() above here as an alternative.

[tejohnson]

In D17982#370625, @tejohnson wrote:

With lazy metadata loading something needs to call materializeMetadata before the IRMover is invoked. Not sure there is an advantage to delaying it in this case, but see the suggestion below for where that could be done.

Rafael, should the IRLinker just always invoke materializeMetadata on the module (if shouldLinkMetadata() is true, which will avoid doing that if we plan to do post-pass metadata linking for ThinLTO)? I'm already invoking materializeMetadata when we detect we are in the postpass md linking. Probably could just move that outside the IsMetadataLinkingPostpass guard in IRMover.cpp. I can send a patch right now to do that, I don't know how to test lld but Sean can give it a try.

See D17992

[• rafael]

Parsing twice is actually a time versus memory tradeoff.

This patch LGTM.

At some point we have to figure out a way of lazy loading metadata and not
copying metadata that is not used, but that is not now.

Cheers,
Rafael

[• rafael]

For now we should just not be lazy. A very big future task is making lazy
loading work so that we don't even read metadata we don't need.

Cheers,
Rafael

[mehdi_amini]

I work on a patch that does that. But because of the way the bitcode format (and the reader) work, it is terribly intrusive.
I ended embedding all metadata in a "flat buffer" container as a pre-requisite for prototyping, and I got good results out of it.
Long term plan is really to move to a new bitcode format IMO.

[• rafael]

Awesome!

ELF/SymbolTable.cpp

 }
 
 static void addBitcodeFile(IRMover &Mover, BitcodeFile &F,
                            LLVMContext &Context) {
   std::unique_ptr<MemoryBuffer> Buffer =
       MemoryBuffer::getMemBuffer(F.MB, false);
   std::unique_ptr<Module> M =
       check(getLazyBitcodeModule(std::move(Buffer), Context,
-                                 /*ShouldLazyLoadMetadata*/ true));
+                                 /*ShouldLazyLoadMetadata*/ false));
   std::vector<GlobalValue *> Keep;
   for (SymbolBody *B : F.getSymbols()) {
     if (B->repl() != B)
       continue;
     auto *DB = dyn_cast<DefinedBitcode>(B);
     if (!DB)
       continue;
     GlobalValue *GV = M->getNamedValue(B->getName());
     assert(GV);
     Keep.push_back(GV);
   }
   Mover.move(std::move(M), Keep, [](GlobalValue &, IRMover::ValueAdder) {});
 }
 
 // Merge all the bitcode files we have seen, codegen the result and return
 // the resulting ObjectFile.
 template <class ELFT>
 ObjectFile<ELFT> *SymbolTable<ELFT>::createCombinedLtoObject() {
   LLVMContext Context;
   Module Combined("ld-temp.o", Context);

test/ELF/lto/metadata.ll

+; REQUIRES: x86
+; RUN: llvm-as %s -o %t1.o
+; RUN: ld.lld -m elf_x86_64 %t1.o %t1.o -o %t.so -shared
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define weak void @foo(i32* %p) {
+  store i32 5, i32* %p, align 4, !tbaa !0
+  ret void
+}
+
+!0 = !{!"Simple C/C++ TBAA"}