[PATCH] D43521: [ThinLTO] Compute synthetic function entry count

[Easwaran Raman via Phabricator via llvm-commits]

eraman marked 3 inline comments as done.
eraman added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:901
+    FunctionSummary *FS =
+        dyn_cast_or_null<FunctionSummary>(Index.findSummaryInModule(
+            F.getGUID(), DestModule.getModuleIdentifier()));
----------------
eraman wrote:
> tejohnson wrote:
> > eraman wrote:
> > > tejohnson wrote:
> > > > Probably shouldn't do this during importing - setting on the functions already in the dest module isn't really related to importing anyway. Also, if done from the caller (i.e. see thinBackend(), we have a ready map of GUID->Summary, DefinedGlobals). Using that is much more efficient that looking up in the full Index hash map.
> > > thinBackend has the DefinedGlobalsMap, but not the other callers to this function. Could you suggest what is the best way to do this on all callers?
> > You only need to invoke this functionality from two places:
> > 1) The new LTO API (thinBackend) - tested via llvm-lto2
> > 2) The old C LTO API (ThinLTOCodeGenerator.cpp) - tested via llvm-lto
> > Both of these already have the DefinedGlobals map, because they invoke other Thin Link optimizations that require it. I.e. search for callers of thinLTOInternalizeModule, which is passed this map. And just add a call to this functionality, split out into a similar type of function, that takes the map. You'll note that for testing purposes, the old C API has the ability to call the thin link optimizations independent of the other optimizations, i.e. see ThinLTOCodeGenerator::internalize, and you may want to do something similar too (for testing via llvm-lto).
> The test I wrote (function_entry_count.ll) runs opt to do function importing. The entry point is from FunctionImportLegacyPass::runOnModule. Is this something that I don't need to support (and rewrite the test appropriately)?
Based on our offline chat, I changed the test not to use opt (and use llvm-lto2 instead) and I am able to move this code as you've suggested.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:975
           return std::move(Err);
+        auto *Summary = Index.findSummaryInModule(GUID, Name);
+        FunctionSummary *FS =
----------------
tejohnson wrote:
> Similarly, this handling can be moved into renameModuleForThinLTO, called further below here, which already does some index-based updates for each imported global.  See the start of processGlobalForThinLTO, which does:
> ValueInfo VI = ImportIndex.getValueInfo(GV.getGUID());
> 
> and the ValueInfo has a pointer to the summary entry. You can have it get the ValueInfo once and share it with your update, which can be done in that method.
> 
> You may want to add an interface to the ValueInfo to get the summary entry given a ModulePath (Name), since it only has an interface to return the list of summary entries currently.
I am not sure what is the good way to guard the change with enable-import-entry-count flag. I don't like the idea of referencing the flag from a utils file, but the  alternative is to pass the value through the callchain starting from renameModuleForThinLTO, which has other callers. Do you have any suggestions?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43521/new/

https://reviews.llvm.org/D43521