[ThinLTO] Fix a metadata lost issue with DICompileUnit import.

[hoy]

Summary

For ThinLTO importing we don't need to import all the fields of the DICompileUnit, such as enums, macros, retained types lists. The importation of those fields were previously disabled by setting their value map entries to nullptr. Unfortunately a metadata node can be shared by multiple metadata operands. Setting the map entry to nullptr might result in not importing other metadata unexpectedly. The issue is fixed by explicitly setting the original DICompileUnit fields (still a copy of the source module metadata) to null.

[wenlei]

LGTM. @tejohnson appreciate if you could take a look too, thanks. (this is blocking an internal usage for us)

llvm/test/ThinLTO/X86/import-metadata.ll

39

Just curious, any reason for !5 = !{!4, !4} instead of !5 = !{!4}?

[tejohnson]

AFAICT this looks ok, but adding @dblaikie as he is more familiar with the DI metadata. Couple minor comments below.

llvm/test/ThinLTO/X86/import-metadata.ll

11

s/metadta/metadata/

15

Should these both be the same MD value? I don't see MD1 matched anywhere else

[hoy]

llvm/test/ThinLTO/X86/import-metadata.ll

15

Actually MD1 comes from the current module. MD2 is from the imported module. We are checking if the imported MD2 doesn't end up having a null operand MD3. Sorry for the confusion. I should make it more explicit.

39

Good point. One !4 is just fine.

[hoy]

Updating D86675: [ThinLTO] Fix a metadata lost issue with DICompileUnit import.

[dblaikie]

In D86675#2252599, @tejohnson wrote:

AFAICT this looks ok, but adding @dblaikie as he is more familiar with the DI metadata. Couple minor comments below.

Thanks - yeah, patch description/changes/patch look right/good to me.

[tejohnson]

LGTM

llvm/lib/Linker/IRMover.cpp

void IRLinker::prepareCompileUnitsForImport() {
   // When importing for ThinLTO, prevent importing of types listed on
   // the DICompileUnit that we don't need a copy of in the importing
   // module. They will be emitted by the originating module.
   for (unsigned I = 0, E = SrcCompileUnits->getNumOperands(); I != E; ++I) {
     auto *CU = cast<DICompileUnit>(SrcCompileUnits->getOperand(I));
     assert(CU && "Expected valid compile unit");
     // Enums, macros, and retained types don't need to be listed on the
     // imported DICompileUnit. This means they will only be imported
-    // if reached from the mapped IR. Do this by setting their value map
-    // entries to nullptr, which will automatically prevent their importing
-    // when reached from the DICompileUnit during metadata mapping.
-    ValueMap.MD()[CU->getRawEnumTypes()].reset(nullptr);
-    ValueMap.MD()[CU->getRawMacros()].reset(nullptr);
-    ValueMap.MD()[CU->getRawRetainedTypes()].reset(nullptr);
+    // if reached from the mapped IR.
+    CU->replaceEnumTypes(nullptr);
+    CU->replaceMacros(nullptr);
+    CU->replaceRetainedTypes(nullptr);
+
     // The original definition (or at least its debug info - if the variable is
-    // internalized an optimized away) will remain in the source module, so
+    // internalized and optimized away) will remain in the source module, so
     // there's no need to import them.
     // If LLVM ever does more advanced optimizations on global variables
     // (removing/localizing write operations, for instance) that can track
     // through debug info, this decision may need to be revisited - but do so
     // with care when it comes to debug info size. Emitting small CUs containing
     // only a few imported entities into every destination module may be very
     // size inefficient.
-    ValueMap.MD()[CU->getRawGlobalVariables()].reset(nullptr);
+    CU->replaceGlobalVariables(nullptr);
 
     // Imported entities only need to be mapped in if they have local
     // scope, as those might correspond to an imported entity inside a
     // function being imported (any locally scoped imported entities that
     // don't end up referenced by an imported function will not be emitted
     // into the object). Imported entities not in a local scope
     // (e.g. on the namespace) only need to be emitted by the originating
     // module. Create a list of the locally scoped imported entities, and
if (ReplaceImportedEntities) {
       if (!AllImportedModules.empty())
         CU->replaceImportedEntities(MDTuple::get(
             CU->getContext(),
             SmallVector<Metadata *, 16>(AllImportedModules.begin(),
                                         AllImportedModules.end())));
       else
         // If there were no local scope imported entities, we can map
         // the whole list to nullptr.
-        ValueMap.MD()[CU->getRawImportedEntities()].reset(nullptr);
+        CU->replaceImportedEntities(nullptr);
     }
   }
 }
 
 /// Insert all of the named MDNodes in Src into the Dest module.
 void IRLinker::linkNamedMDNodes() {
   const NamedMDNode *SrcModFlags = SrcM->getModuleFlagsMetadata();
   for (const NamedMDNode &NMD : SrcM->named_metadata()) {

llvm/test/ThinLTO/X86/Inputs/import-metadata.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-scei-ps4"
+
+define i32 @foo(i32 %goo) {
+entry:
+  %goo.addr = alloca i32, align 4
+  store i32 %goo, i32* %goo.addr, align 4
+  %0 = load i32, i32* %goo.addr, align 4
+  %1 = load i32, i32* %goo.addr, align 4
+  %mul = mul nsw i32 %0, %1
+  ret i32 %mul
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+!llvm.md = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, enums: !4)
+!1 = !DIFile(filename: "foo.cpp", directory: "tmp")
+!2 = !{i32 2, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{}
+!5 = !{!4}

llvm/test/ThinLTO/X86/import-metadata.ll

+; RUN: opt -thinlto-bc %s -o %t1.bc
+; RUN: opt -thinlto-bc %p/Inputs/import-metadata.ll -o %t2.bc
+; RUN: llvm-lto2 run -save-temps %t1.bc %t2.bc -o %t-out \
+; RUN:    -r=%t1.bc,main,plx \
+; RUN:    -r=%t1.bc,foo,l \
+; RUN:    -r=%t2.bc,foo,pl
+; RUN: llvm-dis %t-out.1.3.import.bc -o - | FileCheck %s
+
+;; Check the imported DICompileUnit doesn't have the enums operand.
+;; Also check the imported md metadata that shares a node with the 
+;; enums operand originally is not null.
+
+; CHECK: !llvm.dbg.cu = !{![[#CU1:]], ![[#CU2:]]}
+;; Note that MD1 comes from the current module. MD2 is from the imported module. 
+;; We are checking if the imported MD2 doesn't end up having a null operand.
+; CHECK: !llvm.md = !{![[#MD1:]], ![[#MD2:]]}
+; CHECK: ![[#MD3:]] = !{}
+; CHECK: ![[#CU2]] = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: ![[#FILE2:]], isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug)
+; CHECK: ![[#MD2]] = !{![[#MD3]]}
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-scei-ps4"
+
+declare i32 @foo(i32 %goo)
+
+define i32 @main() {
+  call i32 @foo(i32 0)
+  ret i32 0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+!llvm.md = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, enums: !4)
+!1 = !DIFile(filename: "main.cpp", directory: "tmp")
+!2 = !{i32 2, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{}
+!5 = !{!4}