[PATCH] D136627: [SampleFDO] Compute profile mismatch metrics
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 24 12:55:22 PDT 2022
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
More information about the llvm-commits
mailing list