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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:162
+      CallSiteLoc << " @ ";
+    unsigned int Offset =
+        DIL->getLine() - DIL->getScope()->getSubprogram()->getLine();
----------------
wenlei wrote:
> 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
> ```
> 
So I make sure I understand:
- remarks already output unsigned ints
- negative offsets are possible, due to cases like you mentioned

In this case, I agree that the choice of supporting negative offsets is a separate problem from this CL.

Could you add a comment explaining the current motivation for unsigned it? (i.e. match what remarks do)

Also, could it be uint32_t, to ensure 32 bit-ness?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83743