[PATCH] D147545: [AutoFDO] Stale profile matching(part 2)

[Lei Wang via Phabricator via llvm-commits]

wlei added a comment.

In D147545#4287782 <https://reviews.llvm.org/D147545#4287782>, @hoy wrote:

> How about retitle it as "[AutoFDO] Stale profile matching (part 2)" to align with the first patch? It's mentioned in the summary that only supports CSSPGO for now, so it should be good.

Sounds good.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2304
+        // Force to overwrite the callee name in case any non-call location was
+        // wrote before.
+        IRLocations[IRCallsite] = CalleeName;
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > hoy wrote:
> > > > > wlei wrote:
> > > > > > hoy wrote:
> > > > > > > typo: wrote -> written
> > > > > > > 
> > > > > > > BTW, have you seen such collision? With pseudo probes this shouldn't happen. It should not happen with dwarf discriminators either.
> > > > > > Before I didn't add `isa<IntrinsicInst>(&I)` condition to `(std::optional<PseudoProbe> Probe = extractProbe(I))`
> > > > > > I saw some callee name become empty due to the overwriting by the above emplace.
> > > > > > 
> > > > > > So I was thinking this is to record all the anchors, the anchor should always be higher priority than non-anchor, so I changed like here to force the writing. Also in case any changes when we support AutoFDO or post-link time matching. Or we can use the assertion here.
> > > > > > 
> > > > > I see. Yeah, `extractProbe` on call instruction gets the call probe. If you want block probes only, you can change the check `isa<IntrinsicInst>` to `isa<PseudoProbeInst>`.
> > > > > 
> > > > > 
> > > > For non-call location, the name should be empty. Maybe assert that we never overwrite non-empty name? 
> > > An assert sounds good for pseudo probes. The overwrite is possible for autofdo without enabling dwarf discriminator. 
> > Sounds good, changed to use `isa<PseudoProbeInst>. and added the assert for pseudo probe.
> still seeing `if (FunctionSamples::ProfileIsProbeBased && isa<IntrinsicInst>(&I))` at line 2290.
> 
> LGTM otherwise .
Oops, I missed that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545