[PATCH] D136627: [SampleFDO] Compute profile mismatch metrics

[Lei Wang via Phabricator via llvm-commits]

wlei marked 3 inline comments as done.
wlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:132
 
+static cl::opt<bool> SampleProfileMatching(
+    "sample-profile-fuzzy-match", cl::Hidden, cl::init(false),
----------------
davidxl wrote:
> wenlei wrote:
> > davidxl wrote:
> > > The stats seems like a useful thing independent of fuzzy matching. Should it be controlled with a different option?
> > agreed. something like `-report-profile-staleness`?
> sounds good.
Good point, replaced the `sample-profile-fuzzy-match`, will add it back when fuzzy match is ready.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:431
+  uint64_t TotalCallsiteSamples = 0;
+  uint64_t MismatchedFuncHashSamples = 0;
+  uint64_t TotalFuncHashSamples = 0;
----------------
wenlei wrote:
> How about having another metric to represent how many functions has mismatched profile? That should tell the breath of the mismatch while your current metric tells the severity of mismatch. 
Sounds good.(We already have the same stats(https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1752) but it's only available with the `-stats`).


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2048
 }
+void SampleProfileMatcher::detectProfileMismatch(const Function &F,
+                                                 const FunctionSamples &FS) {
----------------
wenlei wrote:
> nit: white space line in between. 
fixed, thanks!


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2066-2069
+        if (isa<IntrinsicInst>(&I))
+          continue;
+        // If instruction is callbase, match function name with profile
+        if (const auto *CB = dyn_cast<CallBase>(&I)) {
----------------
wenlei wrote:
> Intrinsic is narrower than Call, the checks seem out of order. 
fixed, thanks!


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2086
+          else if (CallsiteFS &&
+                   ((CalleeName.empty() && !CallsiteFS->empty()) ||
+                    (CallsiteFS->find(CalleeName) != CallsiteFS->end())))
----------------
davidxl wrote:
> what does this case cover? inlined indirect call?
Yes, this is to avoid false positives, otherwise all the indirect call function samples will be reported as mismatching, updated the comments. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2098
+    const LineLocation &Loc = I.first;
+    if (Loc.LineOffset & 0x8000)
+      continue;
----------------
davidxl wrote:
> use symbolic constant?
fixed, thanks!


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2131-2134
+  LLVM_DEBUG(
+      dbgs() << "(" << MismatchedFuncHashSamples << "/" << TotalFuncHashSamples
+             << ")"
+             << " of samples are discarded due to function hash mismatch.\n");
----------------
wenlei wrote:
> Is this always going to be 0 for non-probe case? Should we omit this message when probe isn't used? 
Sounds good, made it under the probe condition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136627