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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:368
+
+ContextTrieNode *SampleContextTracker::getContextFor(const DILocation *DIL) {
+  assert(DIL);
----------------
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.  


================
Comment at: llvm/test/Transforms/SampleProfile/profile-context-tracker-debug.ll:1-3
+; REQUIRES: asserts
+; Test for CSSPGO's SampleContextTracker to make sure context profile tree is promoted and merged properly
+; based on inline decision, so post inline counts are accurate.
----------------
wenlei wrote:
> wmi wrote:
> > Is there anything which cannot be tested in profile-context-tracker.ll? The debug message is usually used as last resort if something cannot be fully tested by just checking IR.  
> I added this hoping to make it easier to reason about the internals operations/state of context tracker, and also capture any unintended subtle change in context tracking. But if we look at end result of IR, the non-debug test should be able to cover it as good. I can remove this one if you think that's better.
If there could be unintended subtle change which cannot be caught by the non-debug test, we can keep it. Just make sure the debug messages used in CHECK are all necessary in terms of ensuring the result we are expecting to see. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90125