[PATCH] D159169: [CSSPGO] Refactoring findIRAnchors

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2114
     const Function &F, std::map<LineLocation, StringRef> &IRAnchors) {
+  auto FindTopLevelFrame = [](const DILocation *DIL) {
+    const DILocation *PrevDIL = DIL;
----------------
Maybe assert that `DIL->getInlinedAt()` is not null first. For not inlined DIL, this will not get what we want. Assert helps make it clear. 

With the assert, the loop can also changed to do-while to make it explicit. I find do-while slightly more readable for this kind of cases. 

```
do {
  PrevDIL = DIL;
  DIL = DIL->getInlinedAt();
} while (DIL->getInlinedAt())
```




================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2124
+
+  auto FindCalleeName = [](const CallBase *CB) {
+    StringRef CalleeName = UnknownIndirectCallee;
----------------
nit: this does not involve search/find, so maybe just `GetCanonicalCalleeName`.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2162-2164
+          LineLocation IRCallsite = FunctionSamples::getCallSiteIdentifier(DIL);
+          StringRef CalleeName = FindCalleeName(dyn_cast<CallBase>(&I));
+          IRAnchors.emplace(IRCallsite, CalleeName);
----------------
wlei wrote:
> wlei wrote:
> > wenlei wrote:
> > > Actually can this be unified with `FindTopLevelFrame`? For not inlined calls, `DIL->getSubprogramLinkageName` should be equivalent to `FindCalleeName(dyn_cast<CallBase>(&I))`?
> > Good point, but to clarify:
> > `DIL->getSubprogramLinkageName` is the current/top-level Frame. 
> > `FindCalleeName` is the current callee name, it's the second top Frame.
> > 
> > Maybe my variable name is misleading in the new patch. Here is actually not `FindTopLevelFrame`, For example: "main:1 @ foo", the term "top-level" frame is "main:1", but here what it returns is {1, foo} , i,e {the top-level frame's location, the second top's frame's func name}
> > 
> > uh..I need to fix it.
> > 
> > That said, maybe we still can merge `FindTopLevelFrame` and `FindCalleeName `, like check if it's inlined code inside of the lambda, let me think.
> > 
> OK.. I'm done one version, please take a look https://reviews.llvm.org/D159221
> 
> I ran into some bugs and spent some times debugging, the tricky parts are :
> 
> 1) For pseudo-probe mode, a) for inline code, we need to use `getCallSiteIdentifier` to extract the callsite but b) for non-inlined code, we need to use the probe id.
> 
> 2) The pseudo instruction is a `CallBase` and the `getCalledFunction` will return a non-empty callee name "llvm.pseudoprobe" which actually should be a empty StringRef.
> 
> I then made it work, but as it introduces more complexity, maybe it's still not what we want, any thoughts on this?
> 
> 
I guess, `FindTopLevelFrame` can be called `FindTopLevelInlinedCallsite`? It would also help to explain in a comment what it does with an example, like the `main:1 @ foo` one you mentioned above. 

> For pseudo-probe mode, a) for inline code, we need to use getCallSiteIdentifier to extract the callsite but b) for non-inlined code, we need to use the probe id.

Thanks for clarification - I think that answered my original question, and the answer is no. Getting callsite (loc, callee_name pair) from debug info is only doable for inlined callsite, so for not inlined code, we need to get callee from the call instructions. 

I looked at https://reviews.llvm.org/D159221, i think that with better naming, the version in this patch is actually more readable. wdyt? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159169