[PATCH] D86332: [SampleFDO] Enhance profile remapping support for inline instance

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:218-222
+  // with equivalent CalleeName using remapping rules. To avoid mixing
+  // profile inline instances at one callsite with inline instances at
+  // other callsites, the CalleeRemapper has to be cleared everytime
+  // before it is used, however the remapping table containing the
+  // remapping rules in the CalleeRemapper will be kept in clearing.
----------------
wenlei wrote:
> wmi wrote:
> > wenlei wrote:
> > > wmi wrote:
> > > > wenlei wrote:
> > > > > I guess I didn't fully understand the clearing part - if a function (inlinee) is renamed, why do we not want to apply the remapping globally? E.g. for A->B, and C->B inlined profile, if B is renamed to D, we would want to match D to B for both A->D profile and C->D path, right? Then a global remapper including all names should suffice.
> > > > > 
> > > > > Could you share a concrete example as to why each function needs its own temp remapper for all its inlinees? 
> > > > > 
> > > > Right, given two names B and D, ideally we want the remapper to tell us whether they are identical by returning true or false for remapper.equivalent(B, D). However, remapper doesn't work like this. To check whether B and D are identical, remapper needs to insert B into a foldingset first by parsing it, then check whether D has already existed in it by parsing D and trying to insert D (but not truely insert it). 
> > > > 
> > > > Similarly, if we want to check whether an equivalent name exists in a nameset, remapper need to insert alll the names in the nameset before it can answer the query. So to answer the query whether the callee's name matches an inline instance profile at a specific callsite, it needs to clear names in the remapper inserted at other callsites before it inserts the callee's name at the current callsite into remapper.  
> > > > 
> > > > Existing remapper doesn't have to be cleared throughout the sample profile loading pass because it insert all the names of outline instance in the sample profile, and the question it needs to answer in the whole pass is whether a function name has a match in the outline instances. So because of the way how remapper works, we need different remappers to answer different types of remapping queries. 
> > > Maybe I'm still missing something, but the part that I don't understand is why would we need to have boundary for nameset and the queries? For mangle names, they all live in a single conceptual "namespace", regardless of whether it's name for inlinee or outlined function.
> > > 
> > > For inlinee's name, it's possible that we've inserted their names in global remapper when initializing with all outlined functions' names. If not, we could also insert it into global remapper right before checking for alternative names? To me the only difference for inlinee's name is that, we might discover those names a bit later as we may not see them when initializing the global remapper with outlined function names, but that doesn't seem to warrant creating boundaries for nameset (between outline vs inlinee, inlinees for one functions vs inlinees for another).
> > > why would we need to have boundary for nameset and the queries?
> > 
> > That is because the query we want to have is whether a callee matches any inline instance at a specific callsite location, not the inline stances at any other callsites. That is why we want boundary for nameset related with the current callsite, and need to clear the names related with other callsites. 
> > 
> > > For inlinee's name, it's possible that we've inserted their names in global remapper
> > 
> > Yes, many inline instances have already exist as outline instance in global remapper. If we use global remapper, we can know a callee match an inline/outline instance somewhere in the profile, but don't know whether the callee match an inline instance at the current callsite. 
> Ok, thanks for clarification. Looking closer at the code, I think I understand the reason now. You're iterating over the old names to see if anyone of them matches variant of new name (callee name at the call site), then you have to limit the temp remapper to contain only the new name of interest (callee name). 
> 
> For outlined functions profiles, the remapper provides new_name -> old_profile mapping (`SampleProfileReaderItaniumRemapper::getSamplesFor(StringRef Fname)`). Dealing with name->profile mapping is tricky for inlinee's profiles. But would it be possible to tweak the remapper to support new_name->old_name lookup? With that, inlinee profile can be handled universally by retrieving old name for new callee name, then iterate the call site profiles to get inlinee's profile. 
> 
> And to be consistent with `SampleProfileReaderItaniumRemapper::getSamplesFor` for outline profile, handling of call site profile can also be abstracted away into the remapper? 
> 
> Specifically, I'm thinking about handling inlinees in `SampleProfileReaderItaniumRemapper::applyRemapping` too and keeping a Key to old_name map, instead of Key to samples map.
That is a very good suggestion! Following the suggestion I get the same remapping result with simpler code, and I find I can also add support for indirect call with the simpler implementation and am working on it.  


Repository:
  rL LLVM

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

https://reviews.llvm.org/D86332