This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Import globals recursively
ClosedPublic

Authored by evgeny777 on Jan 30 2020, 5:40 AM.

Diff Detail

Event Timeline

evgeny777 created this revision.Jan 30 2020, 5:40 AM

FYI we had to disable a related variable importing patch due to some widespread order of magnitude increases in thin link time (see D73724). Until that is addressed, this should go under an option off by default, or just held off.

FYI we had to disable a related variable importing patch due to some widespread order of magnitude increases in thin link time (see D73724). Until that is addressed, this should go under an option off by default, or just held off.

I see. Is it possible to reproduce this on some publicly available code base? Like chromium for example

FYI we had to disable a related variable importing patch due to some widespread order of magnitude increases in thin link time (see D73724). Until that is addressed, this should go under an option off by default, or just held off.

I see. Is it possible to reproduce this on some publicly available code base? Like chromium for example

Not sure, I don't know offhand how to build that one. Going to see if I can root cause where the huge increases come from exactly, will get back.

evgeny777 updated this revision to Diff 282230.Jul 31 2020, 8:33 AM

Rebased/improved test case. @tejohnson Can you please look at this? I think that after D73851 compile time regressions are unlikely (at least I don't see them in LLVM test suite)

Rebased/improved test case. @tejohnson Can you please look at this? I think that after D73851 compile time regressions are unlikely (at least I don't see them in LLVM test suite)

I'll take a look today and also give it a try on a large thin link to see the compile time effects.

Rebased/improved test case. @tejohnson Can you please look at this? I think that after D73851 compile time regressions are unlikely (at least I don't see them in LLVM test suite)

I'll take a look today and also give it a try on a large thin link to see the compile time effects.

FYI, on top of D84985, the thin link time for a large binary goes up from 60s to 62s (each an average of 5 runs), which isn't too bad. The number of imported globals increases significantly: 218642 to 363517.

I'll review the code changes sometime today.

tejohnson accepted this revision.Aug 6 2020, 2:11 PM

Sorry this took awhile. LGTM

llvm/lib/Transforms/IPO/FunctionImport.cpp
265

Thanks for cleaning up the unused GUID here - looks like the use was removed in my D48670 patch and I failed to remove it from here.

This revision is now accepted and ready to land.Aug 6 2020, 2:11 PM
This revision was automatically updated to reflect the committed changes.