This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Import only necessary DICompileUnit fields
ClosedPublic

Authored by tejohnson on Dec 9 2016, 3:40 PM.

Details

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.

Event Timeline

tejohnson updated this revision to Diff 80962.Dec 9 2016, 3:40 PM
tejohnson retitled this revision from to [ThinLTO] Import only necessary DICompileUnit fields.
tejohnson updated this object.
tejohnson added a subscriber: llvm-commits.
mehdi_amini edited edge metadata.Dec 9 2016, 4:14 PM

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

include/llvm/Linker/IRMover.h
77

Update the doc

lib/Linker/IRMover.cpp
1012

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

Great to have this assertion and comment now! :)

1040

Why? Can you elaborate in the comment the reason?

lib/Linker/LinkModules.cpp
586

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

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

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 edited edge metadata.Dec 9 2016, 4:39 PM

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

lib/Linker/IRMover.cpp
1015

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

1025

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

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

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 added inline comments.Dec 9 2016, 4:45 PM
test/ThinLTO/X86/debuginfo-cu-import.ll
14

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 added inline comments.Dec 9 2016, 5:00 PM
test/ThinLTO/X86/debuginfo-cu-import.ll
14

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 added inline comments.Dec 9 2016, 5:05 PM
test/ThinLTO/X86/debuginfo-cu-import.ll
14

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

aprantl added inline comments.Dec 9 2016, 5:13 PM
test/ThinLTO/X86/debuginfo-cu-import.ll
14

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 added inline comments.Dec 9 2016, 5:19 PM
test/ThinLTO/X86/Inputs/debuginfo-cu-import.ll
30

Can we strip all the debug info from this file?

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

test3 is confusing here :)

aprantl added inline comments.Dec 10 2016, 10:58 AM
test/ThinLTO/X86/debuginfo-cu-import.ll
20

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.

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

include/llvm/Linker/IRMover.h
77

done

lib/Linker/IRMover.cpp
1012

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

1015

Done

1025

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

1040

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

Done

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

Done

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

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

17

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

20

Fixed

tejohnson updated this revision to Diff 81085.Dec 12 2016, 7:17 AM
tejohnson edited edge metadata.

Address review comments

mehdi_amini accepted this revision.Dec 12 2016, 8:08 AM
mehdi_amini edited edge metadata.

LGTM. Thanks!

lib/Linker/IRMover.cpp
1015

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

1050

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

This revision is now accepted and ready to land.Dec 12 2016, 8:08 AM
This revision was automatically updated to reflect the committed changes.