[PATCH] D48670: [ThinLTO] Ensure we always select the same function copy to import

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:224
+  // i.e. with weak or linkonce linkage).
+  auto *Summary = cast<FunctionSummary>((*It)->getBaseObject());
+  if (Summary->instCount() > Threshold)
----------------
davidxl wrote:
> tejohnson wrote:
> > davidxl wrote:
> > > This changes the behavior in the way that a callee that could be found before this patch may no longer be found. Is this expected?
> > Yes it is a side-effect. We could potentially also change it to only check the prevailing copy (which is typically the first copy anyway), but that would also leave us sometimes without a copy to import whereas we might have imported a different smaller copy previously. 
> > 
> > Another possibility would be to keep a set of functions already imported in to the module and check it first. We do keep the imports currently, but it is by source module, so this would likely need another set to be faster.
> How about fetch all the eligible candidates into a list, sort them by size and pick the first (smallest) one if it is below threshold. This  increases the chances an import will occur.
I'm concerned about the efficiency of doing so - adding the copy and sort to every selectCallee invocation for very large thin links will likely be noticeable. 

I'd lean towards the following, which should fix this issue and also speed up the rejection of callees we have already scanned (after being reached via another caller) and determined were too large. Keep a DenseMap of GUID to processed threshold. Also revert the change here to selectCallee so that we scan all looking for any that is under the threshold. If we decide to import, map the GUID to -1 in the map. If we decide not to import, map GUID to the current threshold in the map. When we encounter a callee, first check the map. If in the map and the threshold is -1, skip as we have already imported it (which fixes the same problem the current version of the patch fixes). If in the map and the mapped threshold is <= the current threshold, don't even bother to scan the summaries (which should add some efficiency on top of the current algorithm). 

This does cost some extra memory, which I can benchmark on a large app. If the memory overhead is noticeable, I can reduce by only doing this this for summary lists that have >1 entry (which means we would do some unnecessary correctness checks on the summary entry if it was already rejected at the current threshold).


Repository:
  rL LLVM

https://reviews.llvm.org/D48670