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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2289
+        if (std::optional<PseudoProbe> Probe = extractProbe(I))
+          IRLocations.emplace(LineLocation(Probe->Id, 0), StringRef());
+      }
----------------
wlei wrote:
> hoy wrote:
> > I guess `IRLocations` should also be populated for non-CS. Maybe add a TODO?
> TODO added, Yes, currently only support pseudo probe.
Looks like we're not populating non-call locations for AutoFDO? In that case, should we assert on CSSPGO to make sure we don't accidentally run this for AutoFDO before its support is complete?


================
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:
> > > 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? 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2262-2264
+  std::set<LineLocation> AllLocations;
+  // Direct call's callee name will be used as the matching anchor.
+  std::unordered_map<LineLocation, StringRef, LineLocationHash> DirectCallSites;
----------------
wlei wrote:
> wenlei wrote:
> > wlei wrote:
> > > wenlei wrote:
> > > > Wondering if we can simplify by merging the two containers, i.e use `std::map<LineLocation, StringRef>`, and empty StringRef value for non-direct-callsite, then we avoid the 2nd map, and also avoid the map look up in `runStaleProfileMatching`. It may use slightly more memory, but feels a bit cleaner. 
> > > > 
> > > > Or even something like `std::map<LineLocation, std::pair<StringRef, bool>>` to have `MatchedCallsiteLocs` merged in as well. These containers feel a bit duplicated.
> > > Good point!  going to remove the `MatchedCallsiteLocs `, so changed to use `std::map<LineLocation, StringRef>` 
> > Looks like MatchedCallsiteLocs is still there? 
> Sorry for confusion, it will also be in a separate diff, same diff with checking big profile change. I plan to use the IRLocations to directly match profile locations, there will be no this `MatchedCallsiteLocs` and use it to guid if the mismatch is big to drop or not. 
Sounds good to deal with it in a separate patch. For `IRLocations`, would be good to have a comment mentioning the use of empty StringRef non-direct-call site. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545