[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:975
           return std::move(Err);
+        auto *Summary = Index.findSummaryInModule(GUID, Name);
+        FunctionSummary *FS =
----------------
tejohnson wrote:
> eraman wrote:
> > 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?
> Perhaps do the updates on both the imported functions and the defined functions (what you were doing above) in renameModuleForThinLTO (it's called on the original module and again below for the imported symbols). Then you cover both my comments (since it's called outside importing for the orig module's defined symbols). In that case the EnableImportEntryCount flag can be completely moved into the utils source file.
I have moved the updates to processGlobalForThinLTO. I am not sure if I am doing the right thing comparing ModulePath() with M.getModuleIdentifier(). 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D43521