[PATCH] D159169: [CSSPGO] Refactoring findIRAnchors

[Lei Wang via Phabricator via llvm-commits]

wlei 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;
----------------
wenlei wrote:
> 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())
> ```
> 
> 
Sounds good, done.


================
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);
----------------
wenlei wrote:
> 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? 
thanks for the suggestion, renamed and updated the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159169