This is an archive of the discontinued LLVM Phabricator instance.

ThinLTO: handles modules with empty summaries
ClosedPublic

Authored by mehdi_amini on Oct 5 2016, 6:28 PM.

Details

Summary

We need to add an entry in the combined-index for modules that have
a hash but otherwise empty summary, this is needed so that we can
get the hash for the module.

Also, if no entry is present in the combined index for a module, we
need to skip it when trying to compute a cache entry.

Event Timeline

mehdi_amini updated this revision to Diff 73717.Oct 5 2016, 6:28 PM
mehdi_amini updated this revision to Diff 73718.
mehdi_amini retitled this revision from to ThinLTO: handles modules with empty summaries.
mehdi_amini updated this object.
mehdi_amini added reviewers: tejohnson, pcc.
mehdi_amini added a subscriber: llvm-commits.

Forgot to git add the test

tejohnson edited edge metadata.Oct 6 2016, 7:45 AM

We need to add an entry in the combined-index for modules that have
a hash but otherwise empty summary, this is needed so that we can
get the hash for the module.

I'm confused because it appears the patch will actually prevent adding an entry in the combined index if the module has an empty summary (ThinLTOCodeGenerator::linkCombinedIndex).

lib/Bitcode/Reader/BitcodeReader.cpp
6151

I think we should just do the addModulePath unconditionally at the top of this routine if we have an index. Then we could assert here that we have module paths. And the other places where we addModulePath would just change to assert/access.

But why is this needed if we are later going to prevent adding this to the combined index since it has an empty summary section?

lib/LTO/LTO.cpp
381

Leftover debugging message

lib/LTO/ThinLTOCodeGenerator.cpp
278

Leftover debugging message.

512

Is a similar change required in LTO::addThinLTO (guarding the call to mergeFrom())?

test/ThinLTO/X86/empty_module_with_cache.ll
14

Why do we have 1 cache entry for llvm-lto2 but 2 for llvm-lto?

tools/llvm-lto/llvm-lto.cpp
393

Leftover debugging message.

Early comments (still working on the rest)

lib/Bitcode/Reader/BitcodeReader.cpp
6151

I think we should just do the addModulePath unconditionally at the top of this routine if we have an index. T

I tried this before, but I believe I ran in trouble because of the combined index: it means we'll add an entry for the combined index in the combined index itself when loading it.

test/ThinLTO/X86/empty_module_with_cache.ll
14

We haven't hooked the cache pruning apparently.

mehdi_amini added inline comments.Oct 6 2016, 1:22 PM
lib/LTO/ThinLTOCodeGenerator.cpp
512

This test was spurious, I removed it.

mehdi_amini updated this revision to Diff 73843.Oct 6 2016, 1:23 PM
mehdi_amini edited edge metadata.

Remove spurious test for empty index

mehdi_amini marked 9 inline comments as done.Oct 6 2016, 1:23 PM

Also, if no entry is present in the combined index for a module, we
need to skip it when trying to compute a cache entry.

The empty summary section just means we won't do any ThinLTO index-based optimization on the module (including importing/exporting). Why can't we still cache it? I.e. with a empty ImportList, ExportList, ResolvedODR, DefinedGlobals?

lib/Bitcode/Reader/BitcodeReader.cpp
6151

Ok that makes sense on why to do it here.

My other question is addressed by the change now added to mergeFrom() and removed from ThinLTOCodeGenerator.cpp, since now we are adding this to the combined index.

lib/LTO/ThinLTOCodeGenerator.cpp
512

Ok, the new comment should be removed then.

test/ThinLTO/X86/empty_module_with_cache.ll
14

I would expect llvm-lto2 to have more files in the cache without pruning. I guess with the current patch we should actually only have 1 since you are preventing caching for the empty module that has an empty summary (although I have a high-level question about that earlier)? So I guess the question is why we still get 2 for llvm-lto?

mehdi_amini updated this revision to Diff 73977.Oct 7 2016, 1:10 PM
mehdi_amini marked an inline comment as done.

Cleanup! (spurious comment/format changes)

Also fix the test: the bitcode didn't have the module hash.

Still have question about why we can't cache if there is no module summary (assuming we do have a hash).

test/ThinLTO/X86/empty_module_with_cache.ll
14

Oh got it, llvm-lto produces the timestamp due to pruning, that is the extra file.

mehdi_amini added inline comments.Oct 7 2016, 4:28 PM
lib/LTO/ThinLTOCodeGenerator.cpp
239

Is your question referring to this line?

If there is no entry for the module, it means it does not have a hash.

test/ThinLTO/X86/empty_module_with_cache.ll
14

The pruning adds a timestamp file.

tejohnson added inline comments.Oct 7 2016, 5:29 PM
lib/LTO/ThinLTOCodeGenerator.cpp
239

Yes and I asked the wrong question, since with this patch modules without a summary will now get a hash and be able to be cached.

So I guess my remaining question is when do we not have an entry here after this patch (as opposed to a 0 valued hash)?

mehdi_amini added inline comments.Oct 7 2016, 8:21 PM
lib/LTO/ThinLTOCodeGenerator.cpp
239

A module with an empty summary and no hash.

tejohnson accepted this revision.Oct 7 2016, 9:43 PM
tejohnson edited edge metadata.

LGTM with new test described below.

lib/LTO/ThinLTOCodeGenerator.cpp
239

Ok, can you add that case to the new test?

This revision is now accepted and ready to land.Oct 7 2016, 9:43 PM
This revision was automatically updated to reflect the committed changes.