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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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:
> 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`?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:431
+  uint64_t TotalCallsiteSamples = 0;
+  uint64_t MismatchedFuncHashSamples = 0;
+  uint64_t TotalFuncHashSamples = 0;
----------------
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. 


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


================
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)) {
----------------
Intrinsic is narrower than Call, the checks seem out of order. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2080-2081
+
+          // Indirect callsite does not have CalleeName, check if callsite has
+          // multiple targets
+          if (CTM && ((CalleeName.empty() && !CTM->empty()) ||
----------------
For readability, restructure it this way? 

```
if (CalleeName.empty())  // indirect call in IR
   ...
else                                  // direct call in IR 
   ...
```


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2083
+          if (CTM && ((CalleeName.empty() && !CTM->empty()) ||
+                      (CTM->find(CalleeName) != CTM->end())))
+            MatchedCallsiteLocs.insert(IRCallsite);
----------------
nit: `CTM->find(CalleeName) != CTM->end()` -> `CTM->count(CalleeName)`


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2087
+                   ((CalleeName.empty() && !CallsiteFS->empty()) ||
+                    (CallsiteFS->find(CalleeName) != CallsiteFS->end())))
+            MatchedCallsiteLocs.insert(IRCallsite);
----------------
nit: `CallsiteFS->find(CalleeName) != CallsiteFS->end()))` -> `CallsiteFS->count(CalleeName)`


================
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");
----------------
Is this always going to be 0 for non-probe case? Should we omit this message when probe isn't used? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136627