This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Remove unused (?) code from thinLTOInternalizeModule
ClosedPublic

Authored by evgeny777 on Dec 7 2017, 10:24 AM.

Details

Summary

I've found the piece of code running on ThinLTO internalization phase which seems useless to me.
Also no test seems to depend on it. I've run check-llvm after removal and it passes flawlessly.

It looks like undefined assembly symbols are first put to GlobalResolutions by lto::addModuleToGlobalRes and than
thinLTOInternalizeAndPromoteInIndex is doing the job by setting the proper linkage in ModuleSummaryIndex.

If this code is needed can someone suggest how to write unit test for it?

Diff Detail

Event Timeline

evgeny777 created this revision.Dec 7 2017, 10:24 AM
evgeny777 added inline comments.Dec 8 2017, 1:54 AM
lib/Transforms/IPO/FunctionImport.cpp
647

The thinLTOInternalizeModule running in separate thread for each module being LTO'ed
So if we found some undefined symbol, which defined in another module what we can do now? That other
module might have already been internalized (or is being internalized at the same moment on another CPU core).

mehdi_amini added inline comments.Dec 8 2017, 9:10 AM
lib/Transforms/IPO/FunctionImport.cpp
647

The comment below says "Can't be internalized if referenced in inline asm", which isn't about cross-module reference. The weird part is that the comment above is supposed to collect "the list of symbols that are not defined in the current module", which shouldn't be possibly interfering with the symbol considered for internalization.

mehdi_amini accepted this revision.Dec 8 2017, 9:16 AM

I wrote this code in April 2016 ( http://reviews.llvm.org/D19103 ), in the very first approach to internalization, which was inferred in the backend and wasn't bullet proof (especially with respect to assembly by the way).
Since then, we moved all decision in the thin-link and this code is indeed (hopefully) dead.

So LGTM! (but it may be a good idea to hear back from Teresa about this)

This revision is now accepted and ready to land.Dec 8 2017, 9:16 AM

@tejohnson Any comments or objections?

tejohnson accepted this revision.Dec 11 2017, 8:50 AM

@tejohnson Any comments or objections?

This should be fine - if we weren't marking these correctly in the summaries before the thin link we would be computing dead symbols incorrectly and have bigger problems.
For gold and lld this information is computed by and accessed via the IRSymtab. I'm not sure / can't remember how this is handled via ld64 and the legacy LTO API, but like I said, thin link dead stripping would overall not work if we didn't mark these as exported.

This revision was automatically updated to reflect the committed changes.