[PATCH] D27635: [ThinLTO] Import only necessary DICompileUnit fields

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

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



================
Comment at: include/llvm/Linker/IRMover.h:77
   ///   for the destination module. It should be true for full LTO, but not
   ///   when importing for ThinLTO, otherwise we can have duplicate symbols.
   Error move(std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
----------------
Update the doc


================
Comment at: lib/Linker/IRMover.cpp:1012
+// When importing for ThinLTO, prevent importing of types listed on
+// the DICompileUnit that we don't need a copy of in the importing
+// module. Do this by setting their value map entries to nullptr,
----------------
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.


================
Comment at: lib/Linker/IRMover.cpp:1036
+               [](const GlobalValue *GV) { return isa<GlobalVariable>(GV); }) &&
+           "Unexpected importing of a GlobalVariable definition");
+    ValueMap.MD()[CU->getRawGlobalVariables()].reset(nullptr);
----------------
Great to have this assertion and comment now! :)


================
Comment at: lib/Linker/IRMover.cpp:1040
+    // Imported entities only need to be mapped in if they have local
+    // scope. Create a list of those, and replace the source CU's
+    // imported entity list with the new list, so only those are mapped in.
----------------
Why? Can you elaborate in the comment the reason?


================
Comment at: lib/Linker/LinkModules.cpp:586
                            },
-                           !isPerformingImport())) {
+                           !isPerformingImport(), isPerformingImport())) {
     handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
----------------
I'd add a comment in front of these parameters for readability


================
Comment at: test/ThinLTO/X86/debuginfo-cu-import.ll:17
+; CHECK: ![[IMP]] = !{![[IMPENTITY:[0-9]+]]}
+; CHECK: ![[IMPENTITY]] = !DIImportedEntity
+
----------------
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.


https://reviews.llvm.org/D27635