[PATCH] D25300: ThinLTO: handles modules with empty summaries

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

> 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?



================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:6151
+            // We always seed the index with the module.
+            TheIndex->addModulePath(Buffer->getBufferIdentifier(), 0);
           if (TheIndex->modulePaths().size() != 1)
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > 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?
> > 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.
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.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:512
+    // If the index is not empty, add it to the combined index
+    if (Index->begin() != Index->end()) {
+      if (CombinedIndex)
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > Is a similar change required in LTO::addThinLTO (guarding the call to mergeFrom())?
> This test was spurious, I removed it.
Ok, the new comment should be removed then. 


================
Comment at: test/ThinLTO/X86/empty_module_with_cache.ll:14
+; RUN:  -r=%t2.bc,_main,plx 
+; RUN: ls %t.cache | count 1
+
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > Why do we have 1 cache entry for llvm-lto2 but 2 for llvm-lto?
> We haven't hooked the cache pruning apparently.
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?


https://reviews.llvm.org/D25300