[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:2106
+    const LineLocation &Loc = I.first;
+    if (Loc.LineOffset & InvalidLineOffsetBit)
+      continue;
----------------
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`?


================
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)) {
----------------
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;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136627