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

Lei Wang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 25 10:34:38 PDT 2022


wlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:308-309
+  size_t operator()(const LineLocation &Loc) const {
+    return std::hash<std::uint64_t>{}((((uint64_t)Loc.LineOffset) << 32) |
+                                      Loc.Discriminator);
+  }
----------------
wenlei wrote:
> It doesn't matter on our platform, but just pointing out that this code isn't portable on host target with size_t -> uint32_t, in which case, the hash becomes just Loc.Discriminator. 
This is unintentional, thanks for pointing this out. changed to `uint64_t`


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2142
+
+  LLVM_DEBUG({
+    if (FunctionSamples::ProfileIsProbeBased) {
----------------
wenlei wrote:
> wlei wrote:
> > wenlei wrote:
> > > When the flag is on, we should always emit the messages, regardless of debug build or not. Same for the one below. 
> > Sounds good! Changed to use `outs`
> I don't know if there's well established convention, but I think most of similar dump go to stderr (i.e. all IR dump goes to stderr). 
Yeah, you're right, dumping into stdout will mess up the IR and make it hard to recompile the IR. Changed to `stderr`


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2106
+    const LineLocation &Loc = I.first;
+    if (Loc.LineOffset & InvalidLineOffsetBit)
+      continue;
----------------
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)?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:431
+  uint64_t TotalCallsiteSamples = 0;
+  uint64_t MismatchedFuncHashSamples = 0;
+  uint64_t TotalFuncHashSamples = 0;
----------------
wenlei wrote:
> wlei wrote:
> > 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`).
> Sorry I wasn't explicit enough, but I was thinking about doing the same for both call site and function. I.e. have both samples mismatch and number of function/callsite mismatch. 
I see, added callsite num.


================
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)) {
----------------
hoy wrote:
> wlei wrote:
> > wenlei wrote:
> > > Intrinsic is narrower than Call, the checks seem out of order. 
> > fixed, thanks!
> nit: maybe more explicit:        
> 
> ```
>      if (!isa<CallInst>(I) && !isa<InvokeInst>(I))
>           continue;
> ```
Tried this, but I found this `!isa<InvokeInst>(I)` doesn't work for the test in this diff. I found other places in the sampleloader are all using the ` isa<IntrinsicInst>(&I)` check, maybe just be consistent with other place?


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