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

[Easwaran Raman via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:847
+
+void llvm::computeSyntheticCounts(ModuleSummaryIndex &Index) {
+  using Scaled64 = ScaledNumber<uint64_t>;
----------------
tejohnson wrote:
> eraman wrote:
> > tejohnson wrote:
> > > I assume that eventually we would want to gate this based on whether we have FDO or not - how will that work?
> > I think eventually we want this to work even when profile data is available by filling in the synthesized count for only those functions which do not have an entry count (a hybrid mode). What I have done now is to gate this on -enable-import-entry-count. To avoid declaring this as extern, I have moved the computeSyntheticCount to FunctionImportUtils.cpp  (where the flag is defined) and bail out if the flag is false. 
> > 
> It doesn't belong in FunctionImportUtils.cpp, which is operating on IR in the backends. It belongs somewhere in lib/LTO since this is done during the LTO/thin link portion of the compile. Historically we have put some of these facilities in FunctionImport.cpp even when they aren't directly related to importing (e.g. computeDeadSymbols), but it probably makes sense for them to move to a separate file under lib/LTO/ that can be shared by both the new and old LTO APIs. Maybe best to just create that new file now for your new code (in which case we can move computeDeadSymbols there as a follow on). Not sure the best name, maybe something like SummaryBasedOptimizations.cpp ? Open to better names...
> 
> Regarding avoiding that option being extern, 2 possibilities come to mind:
> - Does an entry count of 0 in the summary mean that synthetic counts weren't created? If so, can you gate on that.
> Or probably more robust:
> - Consider adding a flag to the index (see withGlobalValueDeadStripping).
I have create a new SummaryBasedOptimizations.cpp and moved the code there. I have also added a flag to Index to indicate whether synthetic counts are computed. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D43521