[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D35081#807497, @mehdi_amini wrote:

> In https://reviews.llvm.org/D35081#807172, @fhahn wrote:
>
> > It's @yunlian, so if the issue you described above covers his failure, than that's great. @tejohnson do you have time to work on a fix soon? Otherwise I could give it a try, I would probably need a few pointers though, as I am not really familiar with ThinLTO.
>
>
> A good starting point is `static const GlobalValueSummary *selectCallee()` in llvm/lib/Transforms/IPO/FunctionImport.cpp ; but it is likely not trivial to solve.


Not necessarily nontrivial from a coding perspective - but the question is how do we want to solve it? E.g. I have a very simple fix that addressed the problem in my test case, by always selecting the first one (assumes that the first is the prevailing, which is generally true).

Should we always select the prevailing copy? E.g. just return the first summary (which is likely to belong to the prevailing copy) if any summary in the list is under the threshold. In my test case and in Yunlian's, we ended up selecting the prevailing copy eventually. But it's possible that only one or more non-prevailing copies will ever be selected. We could just assume that if any copy is under the threshold, that the prevailing is still the best copy to import. But I suppose the prevailing copy could be significantly larger if the inlining during the compile step was very different.

Or, we could always import the copy with the smallest instruction count. But if we have profile data, that profile data likely was collected on the (same) prevailing copy and perhaps that copy was just more aggressively inlined (and might be the better optimized than a non-prevailing copy with a smaller instruction count, which might not have profile data if the early inlining was different leading to the profile not matching when compiling that copy).

Or, we could keep the same selection algorithm, and ensure that we stick with whatever was the first summary selected (in the test case, that was the smaller non-prevailing copy). I.e. keep track of which GUIDs/summaries are already imported for a module, and ensure we don't pick a different one (we do want to reprocess it though if we ever encounter a call to that GUID with a higher threshold, since we may decide to import more of its callees).

Or, we could go back later and see if we have selected more than one summary for a GUID, and pick one (the first one? that's what essentially ends up happening when we have multiple and are doing in-process ThinLTO backends, the IRLinker will likely just keep the first one and ignore the subsequent copies that we try to link in). But that requires some additional post-processing that would be good to avoid if possible.

Thoughts?


https://reviews.llvm.org/D35081