[ThinLTO] Import only necessary DICompileUnit fields

[tejohnson]

Summary

As discussed on mailing list, for ThinLTO importing we don't need
to import all the fields of the DICompileUnit. Don't import enums,
macros, retained types lists. Also only import local scoped imported
entities. Since we don't currently import any global variables,
we also don't need to import the list of global variables (added an
assert to verify none are being imported).

This is being done by pre-populating the value map entries to map
the unneeded metadata to nullptr. For the imported entities, we can
simply replace the source module's list with a new list containing
only those needed imported entities. This is done in the IRLinker
constructor so that value mapping automatically does the desired
mapping.

[mehdi_amini]

This LGTM overall, but wait for someone else (Duncan? Adrian?) to double check! (And see inline comments)

include/llvm/Linker/IRMover.h

77 ↗

(On Diff #80962)

Update the doc

lib/Linker/IRMover.cpp

1012 ↗

(On Diff #80962)

It is strange that the name of the method that is *preventing* importing starts with linkImported..., it seems reverse?
It seems more like preStripImportedCompileUnits() or stripCompileUnitsBeforeImport() or something like that.

1036 ↗

(On Diff #80962)

Great to have this assertion and comment now! :)

1040 ↗

(On Diff #80962)

Why? Can you elaborate in the comment the reason?

lib/Linker/LinkModules.cpp

586 ↗

(On Diff #80962)

I'd add a comment in front of these parameters for readability

test/ThinLTO/X86/debuginfo-cu-import.ll

17 ↗

(On Diff #80962)

This last check seems useless to me: the two imports (!10 and !16) are both !DIImportedEntity. The previous line already checks that one was removed, if you want to verify which one you also need to track the scope.

[aprantl]

As far as I understand it this looks fine/safe to me from the debug info perspective.

lib/Linker/IRMover.cpp

1015 ↗

(On Diff #80962)

not-pick: The coding guidelines want this comment on the declaration in the header file and using ///.

1025 ↗

(On Diff #80962)

Add a comment that they will be precessed when we compile the originating module itself?

test/ThinLTO/X86/debuginfo-cu-import.ll

14 ↗

(On Diff #80962)

I'm not sure this does what we want to. How many DICompileUnits do we expect in the final output? If there is more than one, this will only check the first one.

[mehdi_amini]

test/ThinLTO/X86/debuginfo-cu-import.ll

14 ↗

(On Diff #80962)

It will check that there is no DICompileUnit with any of this field before having a DICompileUnit with an imports field (and we should have only one with imports, and this is the one we don't want the above fields on).

Maybe we could get rid of the debug info in the destination module? That should remove any ambiguity.

[aprantl]

test/ThinLTO/X86/debuginfo-cu-import.ll

14 ↗

(On Diff #80962)

Ok, I wasn't aware of the imports: detail. One way to make the intention clear and unambigous would be to add a CHECK-NOT: DICompileUnit after the CHECK for the CU with the imports.

[mehdi_amini]

test/ThinLTO/X86/debuginfo-cu-import.ll

14 ↗

(On Diff #80962)

That wouldn't be correct, right now we allow the other DICompileUnit to appear later.

[aprantl]

test/ThinLTO/X86/debuginfo-cu-import.ll

14 ↗

(On Diff #80962)

But then this takes us back to my very first comment — that the CHECK-NOTs will only check that we handled the CUs correctly that happen to appear before the CU with the imports?

[mehdi_amini]

test/ThinLTO/X86/Inputs/debuginfo-cu-import.ll

30 ↗

(On Diff #80962)

Can we strip all the debug info from this file?

test/ThinLTO/X86/debuginfo-cu-import.ll

20 ↗

(On Diff #80962)

test3 is confusing here :)

[aprantl]

test/ThinLTO/X86/debuginfo-cu-import.ll

20 ↗

(On Diff #80962)

Ouch :-) Yes, that's exactly what happened. For some reason I assumed that there were 3 DICompileUnits involved and I didn't realize that the other DICompileUnit has none of enums/macros/globals.

[tejohnson]

Thanks, I think I have addressed all of the comments.

include/llvm/Linker/IRMover.h

77 ↗

(On Diff #80962)

done

lib/Linker/IRMover.cpp

1012 ↗

(On Diff #80962)

I changed it to "prepareCompileUnitsForImport". We're mostly setting value map entries to nullptr, so not really stripping it.

1015 ↗

(On Diff #80962)

Done

1025 ↗

(On Diff #80962)

Done (added to header of this loop over the SrcCompileUnits operands)

1040 ↗

(On Diff #80962)

I updated the comment with the rationale as per David Blaikie: This list includes those that are imported entities inside functions, and we need to emit them if the corresponding function is imported. The non-local scope imported entities only need to be emitted in the originating module (e.g. using declarations in namespaces, and namespaces are open and don't need to be complete in every CU anyway).

Added a fixme about eventually restructuring the imported entities metadata, which was David Blaikie's idea for longer term better handling.

lib/Linker/LinkModules.cpp

586 ↗

(On Diff #80962)

Done

test/ThinLTO/X86/Inputs/debuginfo-cu-import.ll

30 ↗

(On Diff #80962)

Done

test/ThinLTO/X86/debuginfo-cu-import.ll

14 ↗

(On Diff #80962)

Addressed by removing debug info from other file. So there is only one DICompileUnit.

17 ↗

(On Diff #80962)

I pared this down to just make sure there is one element remaining in the imported entities list

20 ↗

(On Diff #80962)

Fixed

[tejohnson]

Address review comments

[mehdi_amini]

LGTM. Thanks!

lib/Linker/IRMover.cpp

1018 ↗

(On Diff #81085)

@aprantl: coding standards page says Documentation comments for private APIs can go to the implementation file.

1053 ↗

(On Diff #81085)

@aprantl : the FIXME can be relevant for our work to limit the amount of IR loading.

llvm/trunk/include/llvm/Linker/IRMover.h

public:
   ///   value is referenced by one of the ValuesToLink (transitively) but was
   ///   not present in ValuesToLink. The GlobalValue and a ValueAdder callback
   ///   are passed as an argument, and the callback is expected to be called
   ///   if the GlobalValue needs to be added to the \p ValuesToLink and linked.
   /// - \p LinkModuleInlineAsm is true if the ModuleInlineAsm string in Src
   ///   should be linked with (concatenated into) the ModuleInlineAsm string
   ///   for the destination module. It should be true for full LTO, but not
   ///   when importing for ThinLTO, otherwise we can have duplicate symbols.
+  /// - \p IsPerformingImport is true when this IR link is to perform ThinLTO
+  ///   function importing from Src.
   Error move(std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
              std::function<void(GlobalValue &GV, ValueAdder Add)> AddLazyFor,
-             bool LinkModuleInlineAsm);
+             bool LinkModuleInlineAsm, bool IsPerformingImport);
   Module &getModule() { return Composite; }
 
 private:
   Module &Composite;
   IdentifiedStructTypeSet IdentifiedStructTypes;
   MDMapT SharedMDs; ///< A Metadata map to use for all calls to \a move().
 };
 
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/LTO/LTO.cpp

for (const InputFile::Symbol &Sym :
     }
 
     // FIXME: use proposed local attribute for FinalDefinitionInLinkageUnit.
   }
   assert(ResI == Res.end());
 
   return RegularLTO.Mover->move(Obj->takeModule(), Keep,
                                 [](GlobalValue &, IRMover::ValueAdder) {},
-                                /* LinkModuleInlineAsm */ true);
+                                /* LinkModuleInlineAsm */ true,
+                                /* IsPerformingImport */ false);
 }
 
 // Add a ThinLTO object to the link.
 Error LTO::addThinLTO(std::unique_ptr<InputFile> Input,
                       ArrayRef<SymbolResolution> Res) {
   Module &M = Input->Obj->getModule();
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);

llvm/trunk/lib/Linker/IRMover.cpp

class IRLinker {
   Error linkGlobalValueBody(GlobalValue &Dst, GlobalValue &Src);
 
   /// Functions that take care of cloning a specific global value type
   /// into the destination module.
   GlobalVariable *copyGlobalVariableProto(const GlobalVariable *SGVar);
   Function *copyFunctionProto(const Function *SF);
   GlobalValue *copyGlobalAliasProto(const GlobalAlias *SGA);
 
+  /// When importing for ThinLTO, prevent importing of types listed on
+  /// the DICompileUnit that we don't need a copy of in the importing
+  /// module.
+  void prepareCompileUnitsForImport();
   void linkNamedMDNodes();
 
 public:
   IRLinker(Module &DstM, MDMapT &SharedMDs,
            IRMover::IdentifiedStructTypeSet &Set, std::unique_ptr<Module> SrcM,
            ArrayRef<GlobalValue *> ValuesToLink,
            std::function<void(GlobalValue &, IRMover::ValueAdder)> AddLazyFor,
-           bool LinkModuleInlineAsm)
+           bool LinkModuleInlineAsm, bool IsPerformingImport)
       : DstM(DstM), SrcM(std::move(SrcM)), AddLazyFor(std::move(AddLazyFor)),
         TypeMap(Set), GValMaterializer(*this), LValMaterializer(*this),
         SharedMDs(SharedMDs), LinkModuleInlineAsm(LinkModuleInlineAsm),
         Mapper(ValueMap, RF_MoveDistinctMDs | RF_IgnoreMissingLocals, &TypeMap,
                &GValMaterializer),
         AliasMCID(Mapper.registerAlternateMappingContext(AliasValueMap,
                                                          &LValMaterializer)) {
     ValueMap.getMDMap() = std::move(SharedMDs);
     for (GlobalValue *GV : ValuesToLink)
       maybeAdd(GV);
+    if (IsPerformingImport)
+      prepareCompileUnitsForImport();
   }
   ~IRLinker() { SharedMDs = std::move(*ValueMap.getMDMap()); }
 
   Error run();
   Value *materialize(Value *V, bool ForAlias);
 };
 }
 
Error IRLinker::linkGlobalValueBody(GlobalValue &Dst, GlobalValue &Src) {
   if (auto *GVar = dyn_cast<GlobalVariable>(&Src)) {
     linkGlobalVariable(cast<GlobalVariable>(Dst), *GVar);
     return Error::success();
   }
   linkAliasBody(cast<GlobalAlias>(Dst), cast<GlobalAlias>(Src));
   return Error::success();
 }
 
+void IRLinker::prepareCompileUnitsForImport() {
+  NamedMDNode *SrcCompileUnits = SrcM->getNamedMetadata("llvm.dbg.cu");
+  if (!SrcCompileUnits)
+    return;
+  // 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 we ever start importing global variable defs, we'll need to
+    // add their DIGlobalVariable to the globals list on the imported
+    // DICompileUnit. Confirm none are imported, and then we can
+    // map the list of global variables to nullptr.
+    assert(none_of(
+               ValuesToLink,
+               [](const GlobalValue *GV) { return isa<GlobalVariable>(GV); }) &&
+           "Unexpected importing of a GlobalVariable definition");
+    ValueMap.MD()[CU->getRawGlobalVariables()].reset(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
+    // replace the source CUs imported entity list with the new list, so
+    // only those are mapped in.
+    // FIXME: Locally-scoped imported entities could be moved to the
+    // functions they are local to instead of listing them on the CU, and
+    // we would naturally only link in those needed by function importing.
+    SmallVector<TrackingMDNodeRef, 4> AllImportedModules;
+    bool ReplaceImportedEntities = false;
+    for (auto *IE : CU->getImportedEntities()) {
+      DIScope *Scope = IE->getScope();
+      assert(Scope && "Invalid Scope encoding!");
+      if (isa<DILocalScope>(Scope))
+        AllImportedModules.emplace_back(IE);
+      else
+        ReplaceImportedEntities = true;
+    }
+    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);
+    }
+  }
+}
+
 /// 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()) {
     // Don't link module flags here. Do them separately.
     if (&NMD == SrcModFlags)
       continue;
     NamedMDNode *DestNMD = DstM.getOrInsertNamedMetadata(NMD.getName());
IRMover::IRMover(Module &M) : Composite(M) {
   for (auto *MD : StructTypes.getVisitedMetadata()) {
     SharedMDs[MD].reset(const_cast<MDNode *>(MD));
   }
 }
 
 Error IRMover::move(
     std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
     std::function<void(GlobalValue &, ValueAdder Add)> AddLazyFor,
-    bool LinkModuleInlineAsm) {
+    bool LinkModuleInlineAsm, bool IsPerformingImport) {
   IRLinker TheIRLinker(Composite, SharedMDs, IdentifiedStructTypes,
                        std::move(Src), ValuesToLink, std::move(AddLazyFor),
-                       LinkModuleInlineAsm);
+                       LinkModuleInlineAsm, IsPerformingImport);
   Error E = TheIRLinker.run();
   Composite.dropTriviallyDeadConstantArrays();
   return E;
 }

llvm/trunk/lib/Linker/LinkModules.cpp

bool ModuleLinker::run() {
 
   // FIXME: Propagate Errors through to the caller instead of emitting
   // diagnostics.
   bool HasErrors = false;
   if (Error E = Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(),
                            [this](GlobalValue &GV, IRMover::ValueAdder Add) {
                              addLazyFor(GV, Add);
                            },
-                           !isPerformingImport())) {
+                           /* LinkModuleInlineAsm */ !isPerformingImport(),
+                           /* IsPerformingImport */ isPerformingImport())) {
     handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
       DstM.getContext().diagnose(LinkDiagnosticInfo(DS_Error, EIB.message()));
       HasErrors = true;
     });
   }
   if (HasErrors)
     return true;
 

llvm/trunk/test/ThinLTO/X86/Inputs/debuginfo-cu-import.ll

+; ModuleID = 'debuginfo-cu-import2.c'
+source_filename = "debuginfo-cu-import2.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define i32 @main() {
+entry:
+  call void (...) @foo()
+  ret i32 0
+}
+
+declare void @foo(...) #1

llvm/trunk/test/ThinLTO/X86/debuginfo-cu-import.ll

+; Test to ensure only the necessary DICompileUnit fields are imported
+; for ThinLTO
+
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/debuginfo-cu-import.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc %t2.bc
+
+; Don't import enums, macros, retainedTypes or globals lists.
+; Only import local scope imported entities.
+; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t.index.bc -o - | llvm-dis -o - | FileCheck %s
+; CHECK-NOT: DICompileUnit{{.*}} enums:
+; CHECK-NOT: DICompileUnit{{.*}} macros:
+; CHECK-NOT: DICompileUnit{{.*}} retainedTypes:
+; CHECK-NOT: DICompileUnit{{.*}} globals:
+; CHECK: DICompileUnit{{.*}} imports: ![[IMP:[0-9]+]]
+; CHECK: ![[IMP]] = !{!{{[0-9]+}}}
+
+; ModuleID = 'debuginfo-cu-import.c'
+source_filename = "debuginfo-cu-import.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define void @foo() #0 !dbg !31 {
+entry:
+    ret void, !dbg !34
+}
+
+define void @_ZN1A1aEv() #0 !dbg !4 {
+entry:
+  ret void, !dbg !14
+}
+
+define internal void @_ZN1A1bEv() #0 !dbg !8 {
+entry:
+  ret void, !dbg !15
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!11, !12}
+!llvm.ident = !{!13}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 4.0.0 (trunk 286863) (llvm/trunk 286875)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, macros: !26, retainedTypes: !35, globals: !37, imports: !9)
+!1 = !DIFile(filename: "a2.cc", directory: "")
+!2 = !{!23}
+!4 = distinct !DISubprogram(name: "a", linkageName: "_ZN1A1aEv", scope: !5, file: !1, line: 7, type: !6, isLocal: false, isDefinition: true, scopeLine: 7, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !30)
+!5 = !DINamespace(name: "A", scope: null, file: !1, line: 1)
+!6 = !DISubroutineType(types: !7)
+!7 = !{null}
+!8 = distinct !DISubprogram(name: "b", linkageName: "_ZN1A1bEv", scope: !5, file: !1, line: 8, type: !6, isLocal: true, isDefinition: true, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !30)
+!9 = !{!10, !16}
+!10 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !5, entity: !4, line: 8)
+!11 = !{i32 2, !"Dwarf Version", i32 4}
+!12 = !{i32 2, !"Debug Info Version", i32 3}
+!13 = !{!"clang version 4.0.0 (trunk 286863) (llvm/trunk 286875)"}
+!14 = !DILocation(line: 7, column: 12, scope: !4)
+!15 = !DILocation(line: 8, column: 24, scope: !8)
+!16 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !17, entity: !19, line: 8)
+!17 = distinct !DILexicalBlock(scope: !18, file: !1, line: 9, column: 8)
+!18 = distinct !DISubprogram(name: "c", linkageName: "_ZN1A1cEv", scope: !5, file: !1, line: 9, type: !6, isLocal: false, isDefinition: true, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !30)
+!19 = distinct !DILexicalBlock(scope: !20, file: !1, line: 10, column: 8)
+!20 = distinct !DISubprogram(name: "d", linkageName: "_ZN1A1dEv", scope: !5, file: !1, line: 10, type: !6, isLocal: false, isDefinition: true, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !30)
+!21 = !DILocation(line: 9, column: 8, scope: !18)
+!22 = !DILocation(line: 10, column: 8, scope: !20)
+!23 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "enum1", scope: !5, file: !1, line: 50, size: 32, elements: !30, identifier: "_ZTSN9__gnu_cxx12_Lock_policyE")
+!24 = !{!25}
+!25 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "StateTag", scope: !5, file: !1, line: 1653, size: 32, elements: !30, identifier: "_ZTSN2v88StateTagE")
+!26 = !{!27}
+!27 = !DIMacroFile(line: 0, file: !1, nodes: !28)
+!28 = !{!29}
+!29 = !DIMacro(type: DW_MACINFO_define, line: 3, name: "X", value: "5")
+!30 = !{}
+!31 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !32, isLocal: false, isDefinition: true, scopeLine: 2, isOptimized: false, unit: !0, variables: !30)
+!32 = !DISubroutineType(types: !33)
+!33 = !{null}
+!34 = !DILocation(line: 3, column: 1, scope: !31)
+!35 = !{!36}
+!36 = !DICompositeType(tag: DW_TAG_structure_type, name: "Base", line: 1, size: 32, align: 32, file: !1, elements: !30, identifier: "_ZTS4Base")
+!37 = !{!38}
+!38 = !DIGlobalVariable(name: "version", scope: !5, file: !1, line: 2, type: !36, isLocal: false, isDefinition: true)