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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2167
+    const auto &CTM = I.second.getCallTargets();
+    if (CTM.size() == 1) {
+      StringRef CalleeName = CTM.begin()->first();
----------------
Please add a comment for this. Basically we are filtering out possible indirect calls.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2192
+// Matching heuristic: Match the anchor(with same name) from left to right,
+// first come first matched, non-anchor location match is based on the offset to
+// the last matched anchor.
----------------
May be worth mentioning the matching is based on sequential lexical order. "First come" means the anchors with lexically early locations.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2268
     for (auto &I : BB) {
+      if (std::optional<PseudoProbe> Probe = extractProbe(I)) {
+        AllLocations.insert({Probe->Id, 0});
----------------
Should this be conditioned under `FunctionSamples::ProfileIsProbeBased` if the same is going to be used for non-CS profile?

Also if profile does match, there's no need to populate `AllLocations`?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2269
+      if (std::optional<PseudoProbe> Probe = extractProbe(I)) {
+        AllLocations.insert({Probe->Id, 0});
+      }
----------------
nit: use emplace


================
Comment at: llvm/test/Transforms/SampleProfile/Inputs/pseudo-probe-stale-profile-matching.prof:7
+ 5: 0
+ 7: 124 bar:124
+ 9: 126 bar:126
----------------
Add a case for multiple call targets at same location?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147545