[PATCH] D158817: [CSSPGO] Refactoring SampleProfileMatcher::runOnFunction

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2128
+      if (FunctionSamples::ProfileIsProbeBased && isa<PseudoProbeInst>(&I)) {
+        if (std::optional<PseudoProbe> Probe = extractProbe(I))
+          IRAnchors.emplace(LineLocation(Probe->Id, 0), StringRef());
----------------
wlei wrote:
> hoy wrote:
> > `extractProbe` can also be used to extract a probe from a callsite. Can this code be unified with the callsite handling code below to avoid the overwrite?
> Try to understand this, do you mean we can just use the `LineLocation(Probe->Id, 0)` as the callsite location, it then can replace the `LineLocation IRCallsite = FunctionSamples::getCallSiteIdentifier(DLoc);`?
> 
> If so, that's a good idea for pseudo-probe. 
> 
> However, currently  callsite handling part of the code are still used for AutoFDO, we use the calliste related staleness metrics for AutoFDO. Then we probably still need the callsite handling code.
I think you can just do this

```

   if (FunctionSamples::ProfileIsProbeBased) {
        if (auto Probe = extractProbe(I)) {
            StringRef CalleeName;
            if (const auto *CB = dyn_cast<CallBase>(&I)) {
                if (Function *Callee = I.getCalledFunction()){
                    CalleeName = FunctionSamples::getCanonicalFnName(Callee->getName());
               } else {
                  CalleeName = UnknownIndirectCallee;
               }
            }
            IRAnchors.emplace(LineLocation(Probe->Id, 0), CalleeName);
       }
   }
```

Let me know if it works. As for AutoFDO, we could probably leave it a TODO just as what you do now and redesign the whole logic later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158817