This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Import local variables from the same module as caller
ClosedPublic

Authored by tejohnson on Nov 28 2018, 10:19 PM.

Details

Summary

We can sometimes end up with multiple copies of a local variable that
have the same GUID in the index. This happens when there are local
variables with the same name that are in different source files having the
same name/path at compile time (but compiled into different bitcode objects).

In this case make sure we import the copy in the caller's module.
This enables importing both of the variables having the same GUID
(but which will have different promoted names since the module paths,
and therefore the module hashes, will be distinct).

Importing the wrong copy is particularly problematic for read only
variables, since we must import them as a local copy whenever
referenced. Otherwise we get undefs at link time.

Note that the llvm-lto.cpp and ThinLTOCodeGenerator changes are needed
for testing the distributed index case via clang, which will be sent as
a separate clang-side patch shortly. We were previously not doing the
dead code/read only computation before computing imports when testing
distributed index generation (like it was for testing importing and
other ThinLTO mechanisms alone).

Diff Detail

Event Timeline

tejohnson created this revision.Nov 28 2018, 10:19 PM
evgeny777 added inline comments.Nov 29 2018, 4:41 AM
lib/Transforms/IPO/FunctionImport.cpp
304

Why do you need to check for VI.getSummaryList().size() > 1? New test cases are passing without it.

tejohnson marked an inline comment as done.Nov 29 2018, 6:21 AM
tejohnson added inline comments.
lib/Transforms/IPO/FunctionImport.cpp
304

It's probably unnecessary in this context. I had copied this handling from similar code for functions in selectCallee, but there you can have references to locals in another module via indirect call profiles which doesn't apply here. I can go ahead and remove it here.

tejohnson updated this revision to Diff 175863.Nov 29 2018, 6:24 AM

Address comment

This revision is now accepted and ready to land.Nov 29 2018, 7:18 AM
This revision was automatically updated to reflect the committed changes.