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

[Easwaran Raman via Phabricator via llvm-commits]

eraman marked 5 inline comments as done.
eraman added a comment.
Herald added subscribers: dang, arphaman, dexonsmith, steven_wu.

Sorry for this very long delay in following up.



================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:901
+    FunctionSummary *FS =
+        dyn_cast_or_null<FunctionSummary>(Index.findSummaryInModule(
+            F.getGUID(), DestModule.getModuleIdentifier()));
----------------
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)?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:729
+  // be propagated across the combined callgraph.
+  // SyntheticCountsUtils::propagate takes of this propagation on any callgraph
+  // that specialized GraphTraits.
----------------
tejohnson wrote:
> Nit: comment needs a fix, i.e. should "takes of" be "takes on"? 
I think I intended "takes care of"


Repository:
  rL LLVM

https://reviews.llvm.org/D43521