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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks, I think I have addressed all of the 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,
----------------
mehdi_amini wrote:
> Update the doc
done


================
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,
----------------
mehdi_amini wrote:
> 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.
I changed it to "prepareCompileUnitsForImport". We're mostly setting value map entries to nullptr, so not really stripping it.


================
Comment at: lib/Linker/IRMover.cpp:1015
+// which will automatically prevent their importing when reached
+// from the DICompileUnit during metadata mapping.
+void IRLinker::linkImportedCompileUnits() {
----------------
aprantl wrote:
> not-pick: The coding guidelines want this comment on the declaration in the header file and using `///`.
Done


================
Comment at: lib/Linker/IRMover.cpp:1025
+    // imported DICompileUnit. This means they will only be imported
+    // if reached from the mapped IR.
+    ValueMap.MD()[CU->getRawEnumTypes()].reset(nullptr);
----------------
aprantl wrote:
> Add a comment that they will be precessed when we compile the originating module itself?
Done (added to header of this loop over the SrcCompileUnits operands)


================
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.
----------------
mehdi_amini wrote:
> Why? Can you elaborate in the comment the reason?
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.


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


================
Comment at: test/ThinLTO/X86/Inputs/debuginfo-cu-import.ll:30
+!10 = !DILocation(line: 3, column: 3, scope: !6)
+!11 = !DILocation(line: 4, column: 1, scope: !6)
+
----------------
mehdi_amini wrote:
> Can we strip all the debug info from this file?
Done


================
Comment at: test/ThinLTO/X86/debuginfo-cu-import.ll:14
+; CHECK-NOT: DICompileUnit{{.*}} retainedTypes:
+; CHECK-NOT: DICompileUnit{{.*}} globals:
+; CHECK: DICompileUnit{{.*}} imports: ![[IMP:[0-9]+]]
----------------
aprantl wrote:
> mehdi_amini wrote:
> > aprantl wrote:
> > > mehdi_amini wrote:
> > > > aprantl wrote:
> > > > > 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.
> > > > 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.
> > > 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.
> > That wouldn't be correct, right now we allow the other DICompileUnit to appear later.
> 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?
Addressed by removing debug info from other file. So there is only one DICompileUnit.


================
Comment at: test/ThinLTO/X86/debuginfo-cu-import.ll:17
+; CHECK: ![[IMP]] = !{![[IMPENTITY:[0-9]+]]}
+; CHECK: ![[IMPENTITY]] = !DIImportedEntity
+
----------------
mehdi_amini wrote:
> 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.
I pared this down to just make sure there is one element remaining in the imported entities list


================
Comment at: test/ThinLTO/X86/debuginfo-cu-import.ll:20
+; ModuleID = 'test3.c'
+source_filename = "test3.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
aprantl wrote:
> mehdi_amini wrote:
> > test3 is confusing here :)
> 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.
Fixed


https://reviews.llvm.org/D27635