[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:
> > 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.
> I guess we even don't need the FunctionSamples::ProfileIsProbeBased. 

It should still be useful. In theory, we can still use AutoFDO profile for a probed function. Actually our dual-profile mode is just a use case. 

Sounds good to make it a new patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158817