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

[Lei Wang via Phabricator via llvm-commits]

wlei 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());
----------------
hoy wrote:
> 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.
Thanks for the suggestion. That looks cleaner, let me give it a try.

I guess we even don't need the `FunctionSamples::ProfileIsProbeBased`.

Also do you mind I try it in a new patch, on top of the stack, because this patch is the bottom of the patch stack, then I need to more rebasing and then need to request for review for the accepted patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158817