[PATCH] D35876: ThinLTO: Don't import imported entities (not even local ones)

[David Blaikie via Phabricator via llvm-commits]

dblaikie created this revision.
Herald added subscribers: eraman, inglorion, aprantl, mehdi_amini.

This was my mistake/bad advice when this feature was being implemented -
currently local imported entities are only attached to function
definitions - and definitions are created (even though there's no code
for them) in the DWARF to attach the imported entity to if no definition
exists otherwise.

So importing the imported entities when the only thing that's going to
be produced is inline definitions is unnecessary & also leads to
creationg of extra CUs to house the synthesized definition which is
otherwise unused.

At some point the imported entity work could be improved on, to (as the
old comment mentions) move local imported entities to be attached to the
function/subprogram so they would eb dropped/imported naturally - but
also to ensure the imported entities go on the appropriate entities
(maybe even on the inlined subroutine, or on the abstract origin(s)
(there may be multiple abstract origins when doing Fission(especially
DWP)+LTO because cross-CU references aren't possible there))


https://reviews.llvm.org/D35876

Files:
  lib/Linker/IRMover.cpp
  test/ThinLTO/X86/debuginfo-cu-import.ll


Index: test/ThinLTO/X86/debuginfo-cu-import.ll
===================================================================
--- test/ThinLTO/X86/debuginfo-cu-import.ll
+++ test/ThinLTO/X86/debuginfo-cu-import.ll
@@ -8,12 +8,13 @@
 ; 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]+}}}
+; CHECK: DICompileUnit(
+; CHECK-NOT: enums:
+; CHECK-NOT: macros:
+; CHECK-NOT: retainedTypes:
+; CHECK-NOT: globals:
+; CHECK-NOT: imports:
+; CHECK-SAME: FullDebug){{$}}
 
 ; ModuleID = 'debuginfo-cu-import.c'
 source_filename = "debuginfo-cu-import.c"
Index: lib/Linker/IRMover.cpp
===================================================================
--- lib/Linker/IRMover.cpp
+++ lib/Linker/IRMover.cpp
@@ -1023,14 +1023,15 @@
   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
+    // Enums, macros, retained types, and imported entities 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);
+    ValueMap.MD()[CU->getRawImportedEntities()].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
@@ -1040,40 +1041,6 @@
                [](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);
-    }
   }
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D35876.108206.patch
Type: text/x-patch
Size: 4428 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170726/91a51f57/attachment.bin>