[PATCH] D90125: [CSSPGO] Infrastructure for context-sensitive Sample PGO and Inlining

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:368
+
+ContextTrieNode *SampleContextTracker::getContextFor(const DILocation *DIL) {
+  assert(DIL);
----------------
wmi wrote:
> wenlei wrote:
> > wmi wrote:
> > > Do we need to call getCanonicalFnName here to make the name in inline stack canonical so we can match the name in inline stack with the name in context?
> > Good catch, I think we need to canonicalize `CalleeName` which is the leaf. (The names of middle inline frames should be fine as they're from debug metadata which are not modified when suffixes are appended for symbol promotion, etc..)
> > 
> > I guess we need to add `getCanonicalFnName` for `SampleProfileLoader::findCalleeFunctionSamples`. IIUC, we need it there for today's FDO too?
> In the context, there are also levels contributed by stack unwinding. Those frames should have the same names as elf symbols. To be consistent, do we want to apply getCanonicalFnName for all the context levels?   
> 
> > I guess we need to add getCanonicalFnName for SampleProfileLoader::findCalleeFunctionSamples. IIUC, we need it there for today's FDO too?
> 
> Agree. Today, it may not need it because most suffixes are appended after inline so like you said the names of the inline frames from debug metadata don't contain the suffixes. But there are now suffixes being added before inline (https://reviews.llvm.org/D89617) and there may be others in the future. It is good to always apply the function.  
> In the context, there are also levels contributed by stack unwinding. Those frames should have the same names as elf symbols. To be consistent, do we want to apply getCanonicalFnName for all the context levels? 

Good point. In this case, I think it's better canonicalize all names during profile generation though. IIRC AutoFDO get names from dwarf hence it does not have the suffixes (as if it's canonicalized). So doing canonicalization during profile generation would make it consistent with AutoFDO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90125