[PATCH] D94333: [Inliner] Change inline remark format

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.

In D94333#2487829 <https://reviews.llvm.org/D94333#2487829>, @davidxl wrote:

> The proposal in the patch is to use line:col.discriminator (which is reasonable), but the implementation uses line:col:discriminator -- please update the patch to be consistent.

I might have missed something but the implemented format is line:col.discriminator. See:

remark: calls.cc:4:0: _Z3subii inlined into main to match profiling context with (cost=-5, threshold=337) at callsite _Z3sumii:1:0 @ **main:3:0.1;**

and

  if (Discriminator)
    CallSiteLoc << "." << llvm::utostr(Discriminator);



================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:418
       Name = DIL->getScope()->getSubprogram()->getName();
-    Remark << Name << ":" << ore::NV("Line", Offset);
+    Remark << Name << ":" << ore::NV("Line", Offset) << ":"
+           << ore::NV("Column", DIL->getColumn());
----------------
wenlei wrote:
> thegameg wrote:
> > Can we use a `DebugLoc` here like `Callee` and `Caller`?
> > 
> > Something like:
> > 
> > ```
> > - String: 'at callsite '
> > - CallSite: bar
> >   DebugLoc: { File: '/tmp/s.c', Line: 1, Column: 10 }
> > ```
> Having location in text output as well (in addition to yaml) makes it easier to consume for replay. If we go with what Caller/Callee use, I guess the location only goes to yaml, but not text remarks?
> 
> As for the format of location string in text mark, the current form aligns with the context representation of CSSPGO's profile, so it makes things a bit easier for inlining related investigation, etc, which is intended use case for inline replay scaffolding.
I like the idea! Testing out using the DLoc as an ORE generates:
```
  - CallSite:        'calls.cc:10:0'
    DebugLoc:        { File: calls.cc, Line: 10, Column: 0 }
```
 As compared to:
```
  - String:          main
  - String:          ':'
  - Line:            '3'
  - String:          ':'
  - Column:          '0'
  - String:          .
  - Disc:            '1'
  - String:          ';'
```

We can probably package up the DLoc to output its name better like how Callee gets it automatically with `ore::NV("Callee", &Callee)`. One of the refinements we do here though is to generate function-relative line numbers which are more tolerant of adding/deleting source lines while maintaining inline replay. Also, looking at the implementation DebugLoc doesn't seem to recurse through inlined sites which we need for cases like:

`callsite _Z3sumii:1:0 @ main:3:0.1;`

If we can extend DebugLoc to have the processed form than I think that's an elegant way to do this.

Additionally I've verified that DebugLoc only shows up in the yaml output which with the current scheme of reading the remarks as plaintext means that won't work. There's discussion on companion diff D94334 on whether it makes more sense to use YAML for these remarks which also informs whether we should go down this route.


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

https://reviews.llvm.org/D94333