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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2154
+  if (FunctionSamples::ProfileIsProbeBased) {
+    dbgs() << "(" << NumMismatchedFuncHash << "/" << TotalProfiledFunc << ")"
+           << " of functions' profile are invalid and "
----------------
Still seeing ` dbgs()` here. Should it be `stderr`?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2106
+    const LineLocation &Loc = I.first;
+    if (Loc.LineOffset & InvalidLineOffsetBit)
+      continue;
----------------
wlei wrote:
> wenlei wrote:
> > hoy wrote:
> > > wlei wrote:
> > > > hoy wrote:
> > > > > Should invalid line offset also be considered mismatched samples? The samples will discarded any way. 
> > > > > 
> > > > > BTW, sort of having an impression that the invalid line offset checking is used anywhere else. Can we unify the usage if that's the case?
> > > > From my observation, the invalid(negative) offset will never be matched to any offset in the IR, so we actually need to remove it from the profile rather than report as mismatching. Recalling we tried to remove this offset in llvm-profgen, however, it caused a regression(likely because it affect the sample hot cutoff), so we chose to keep it. As for the mismatch,  I'm thinking to report the samples that's caused by the real stale profile issue, this might mislead the user. 
> > > > 
> > > > > BTW, sort of having an impression that the invalid line offset checking is used anywhere else. Can we unify the usage if that's the case?
> > > > yeah, that's from llvm-profgen, but that diff is reverted.
> > > Sounds good to exclude invalid line offsets.
> > > 
> > > 
> > > > yeah, that's from llvm-profgen, but that diff is reverted.
> > > 
> > > I see. Factor out the checking logic here to be a general function, maybe a static function of `LineLocation`?
> > IIRC, these represents sample that can't be attributed to a particular line, but excluding them in profile generation changes total sample of a function, which led to regression. 
> > 
> > I think excluding them in mismatch count makes sense because there's not much user can do about these, i.e. a profile refresh won't make them go away. 
> > I see. Factor out the checking logic here to be a general function, maybe a static function of LineLocation?
> Sorry I meant the usage in llvm-profgen is removed and now here is the only place to use this check, I'm not very sure if this is a standard way to check if offset is valid(to make it in `LineLocation`), so how about just leave it here(refactor when other places used it in the future)?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136627