[PATCH] D156722: [CSSPGO] Support stale profile matching for LTO

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2117
 
+void SampleProfileMatcher::populateIRLocations(
+    const Function &F, const FunctionSamples &FS,
----------------
wlei wrote:
> wenlei wrote:
> > wenlei wrote:
> > > nit on function naming: it not only populates `IRLocations`, but also `MatchedCallsiteLocs`.
> > More like `findAnchorLocations`?
> Sounds good to move `MatchedCallsiteLocs ` out of `populateIRLocations`, I also saw you mentioned to break the functions in other patch, I will do the refactoring in a new diff.
> 
> Regarding the name `findAnchorLocations`, we use the term "Anchor" for both IR and Profile, see in the `runStaleProfileMatching`, we use the `ProfileAnchors`. So maybe still keep `populateIRlocations` or `findIRLocations`? after the refactoring, it should only be related with IR.
Assuming the part that deals with profile will be moved out of this function ,how about let's name it as `findIRAnchors`? 

In `runStaleProfileMatching`, we find `ProfileAnchors` and match them with `IRAnchors` established here.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2377
   // The value of the map is the name of direct callsite and use empty StringRef
   // for non-direct-call site.
   std::map<LineLocation, StringRef> IRLocations;
----------------
wenlei wrote:
> As I read the code I noticed that "non-direct-call site" can be confusing as it can mean call site that is not direct call. But we really mean to say is 1) not a call site, 2) indirect call site? 
Can you update the comments to avoid ambiguity of "non-direct-call site"? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156722