Fix assertions on lazy-loading of Metadata TBAA attachments

[mehdi_amini]

Summary

The issue happens with:

%0 = ....., !tbaa !0
%1 = ....., !tbaa !1

With !0 that references !1.

In this case when loading !0 we generates a temporary for the
operand !1. We now flush it immediately and trigger the load of
!1 before moving on. If we don't we get the temporary when
attaching to %1. This is usually not an issue except that we
eagerly try to update TBAA MDNodes, which is obviously not possible
if we only have a temporary.

[tejohnson]

LGTM as a bug fix. But I have a few general questions below.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

704 ↗

(On Diff #83466)

I just noticed that Placeholders is never used in lazyLoadModuleMetadataBlock, so it should probably be removed (in a follow up patch). It does create forward refs so I guess resolveForwardRefsAndPlaceholders below still needs to be invoked, but should it be invoked from within lazyLoadModuleMetadataBlock instead (and passed a dummy PlaceholdersQueue)?

1613 ↗

(On Diff #83466)

I suppose there is no loss in doing this more frequently since we know we need to resolve these anyway? Is there any disadvantage to doing the resolveForwardRefsAndPlaceholders more frequently? If so, should it only be done for TBAA attachments? If not, then should we be doing it more frequently elsewhere?

[mehdi_amini]

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

1613 ↗

(On Diff #83466)

I'm looking into this, especially because since committing the lazy-loading I don't reproduce the improvements I had on my early version. I likely fixed a bug between my prototype and the final version.
I have >10% of the *total* link time spent in RAUW MDNodes.

llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp

 /// Parse metadata attachments.
 Error MetadataLoader::MetadataLoaderImpl::parseMetadataAttachment(
     Function &F, const SmallVectorImpl<Instruction *> &InstructionList) {
   if (Stream.EnterSubBlock(bitc::METADATA_ATTACHMENT_ID))
     return error("Invalid record");
 
   SmallVector<uint64_t, 64> Record;
 
-  PlaceholderQueue Placeholders;
-
   while (true) {
     BitstreamEntry Entry = Stream.advanceSkippingSubblocks();
 
     switch (Entry.Kind) {
     case BitstreamEntry::SubBlock: // Handled for us already.
     case BitstreamEntry::Error:
       return error("Malformed block");
     case BitstreamEntry::EndBlock:
-      resolveForwardRefsAndPlaceholders(Placeholders);
       return Error::success();
     case BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     // Read a metadata attachment record.
     Record.clear();
case bitc::METADATA_ATTACHMENT: {
         DenseMap<unsigned, unsigned>::iterator I = MDKindMap.find(Kind);
         if (I == MDKindMap.end())
           return error("Invalid ID");
         if (I->second == LLVMContext::MD_tbaa && StripTBAA)
           continue;
 
         auto Idx = Record[i + 1];
         if (Idx < (MDStringRef.size() + GlobalMetadataBitPosIndex.size()) &&
-            !MetadataList.lookup(Idx))
+            !MetadataList.lookup(Idx)) {
           // Load the attachment if it is in the lazy-loadable range and hasn't
           // been loaded yet.
+          PlaceholderQueue Placeholders;
           lazyLoadOneMetadata(Idx, Placeholders);
+          resolveForwardRefsAndPlaceholders(Placeholders);
+        }
 
         Metadata *Node = MetadataList.getMetadataFwdRef(Idx);
         if (isa<LocalAsMetadata>(Node))
           // Drop the attachment.  This used to be legal, but there's no
           // upgrade path.
           break;
         MDNode *MD = dyn_cast_or_null<MDNode>(Node);
         if (!MD)

llvm/trunk/test/ThinLTO/X86/Inputs/funcimport-tbaa.ll

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+
+define i32 @main() {
+entry:
+  %unused = call float @globalfunc1(i32* null, float*null)
+  ret i32 0
+}
+
+declare float @globalfunc1(i32*, float*)
 No newline at end of file

llvm/trunk/test/ThinLTO/X86/funcimport-tbaa.ll

+; We generate invalid TBAA, hence -disable-verify, but this is a convenient way
+; to trigger a metadata lazyloading crash
+
+; RUN: opt -module-summary %s -o %t.bc -bitcode-mdindex-threshold=0 -disable-verify
+; RUN: opt -module-summary %p/Inputs/funcimport-tbaa.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
+
+
+; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc -o - \
+; RUN:  | llvm-dis -o - | FileCheck %s --check-prefix=IMPORTGLOB1
+; IMPORTGLOB1: define available_externally float @globalfunc1
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+define float @globalfunc1(i32*, float*) {
+  %3 = load i32, i32* %0, align 4, !tbaa !0
+  %4 = sitofp i32 %3 to float
+  %5 = load float, float* %1, align 4, !tbaa !4
+  %6 = fadd float %4, %5
+  ret float %6
+}
+
+; We need a second function for force the metadata to be emitted in the global block
+define float @globalfunc2(i32*, float*) {
+  %3 = load i32, i32* %0, align 4, !tbaa !0
+  %4 = sitofp i32 %3 to float
+  %5 = load float, float* %1, align 4, !tbaa !4
+  %6 = fadd float %4, %5
+  ret float %6
+}
+
+!0 = !{!1, !4, i64 0}
+!1 = !{!"int", !2, i64 0}
+!2 = !{!"omnipotent char", !3, i64 0}
+!3 = !{!"Simple C/C++ TBAA"}
+!4 = !{!5, !5, i64 0}
+!5 = !{!"float", !2, i64 0}