[PATCH] D83743: [InlineAdvisor] New inliner advisor to replay inlining from optimization remarks

[Wenlei He via Phabricator via llvm-commits]

wenlei marked an inline comment as done.
wenlei added inline comments.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:162
+      CallSiteLoc << " @ ";
+    unsigned int Offset =
+        DIL->getLine() - DIL->getScope()->getSubprogram()->getLine();
----------------
mtrofin wrote:
> wenlei wrote:
> > mtrofin wrote:
> > > this assumes DIL->getLine() >= DIL->getScope()->getSubprogram()->getLine(). Perhaps an assert before, or Offset could be int, and check it's non-negative?
> > This is consistent with `addLocationToRemarks` where we construct the location string for remarks, so the output can be consumed without change. Negative line offset is possible, we've seen that in FDO profile, and they're encoded with unsigned int there too - I don't know why that's the case, but seems there's convention and functionality-wise it works as long as everything is consistent..
> I don't think that makes it right, though. I'd suggest at least having a test here, then, i.e. bailing out if the this is greater than lhs. I'll look into the other API.
I'm not sure if we want to bail out on negative offsets. I added `addLocationToRemarks` couple weeks back. Unsigned int was used there so we can match line offset from remarks to the line offset from FDO profile for negative offsets, which is important from tooling perspective. 

Ideally we shouldn't have negative offset, but practically, we see it. If we put an assertion there, it will fire. Due to macro, there could be cases where LHS and RHS come from different files, hence the subtraction can lead to negative offset; and there could be other cases due to not having a line number on LHS so 0 is retrieved for LHS. We've been living with that for a while (see `FunctionSamples::getOffset`), and here's an example of FDO profile where negative offset is encoded using unsigned int (the last three lines).

Not advocating for that, but just thought asserting non-negative offset or fixing what's breaking that assertion is a separate work. And being consistent with what we've been doing (e.g. `FunctionSamples::getOffset`) should be good. What do you think?

```
ZSTD_decompressSequences_bmi2:736988668:1677
 0: 1557
 6: 93
 5: ZSTD_decompressSequences_body:736954111
  8: 1557
  9: 1557
  10: 1557
  11: 1557
  53: 109
  55: 109
  58: 0
  64693: 1520
  64940: 1564
  65080: 1264073
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83743