Page MenuHomePhabricator

[Inliner] Change inline remark format and update ReplayInlineAdvisor to use it
ClosedPublic

Authored by modimo on Jan 8 2021, 12:35 PM.

Details

Summary

This change modifies the source location formatting from:
LineNumber.Discriminator
to:
LineNumber:ColumnNumber.Discriminator

The motivation here is to enhance location information for inline replay that currently exists for the SampleProfile inliner. This will be leveraged further in inline replay for the CGSCC inliner in the related diff.

The ReplayInlineAdvisor is also modified to read the new format and now takes into account the callee for greater accuracy.

Testing:
ninja check-llvm

Diff Detail

Event Timeline

modimo created this revision.Jan 8 2021, 12:35 PM
modimo requested review of this revision.Jan 8 2021, 12:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2021, 12:35 PM
modimo retitled this revision from change remark format to [Inliner] Change inline remark format.Jan 8 2021, 12:41 PM
modimo edited the summary of this revision. (Show Details)
modimo added reviewers: mtrofin, wenlei, wmi.
thegameg added inline comments.
llvm/lib/Analysis/InlineAdvisor.cpp
405–406

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 }

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.

wenlei added inline comments.Jan 8 2021, 2:33 PM
llvm/lib/Analysis/InlineAdvisor.cpp
405–406

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.

modimo updated this revision to Diff 315556.Jan 8 2021, 4:58 PM
modimo edited the summary of this revision. (Show Details)

Split changes with D94334 better: now the ReplayInlineAdvisor update which needs to be atomic with the formatting change is moved over to here. Removed ReplayInlineAdvice and modified DefaultInlineAdvice to provide the desired functionality. Fixed up tests.

modimo added a comment.Jan 8 2021, 5:25 PM

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);
llvm/lib/Analysis/InlineAdvisor.cpp
405–406

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.

modimo retitled this revision from [Inliner] Change inline remark format to [Inliner] Change inline remark format and update ReplayInlineAdvisor to it.Jan 8 2021, 5:27 PM
modimo edited the summary of this revision. (Show Details)
modimo retitled this revision from [Inliner] Change inline remark format and update ReplayInlineAdvisor to it to [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it.Jan 8 2021, 5:35 PM

You are right -- the text format is properly implemented.

mtrofin added inline comments.Jan 8 2021, 7:04 PM
llvm/lib/Analysis/InlineAdvisor.cpp
65

Why add optional remark emission?

Should we get rid of DefaultInlineAdvice and fold it into the base class InlineAdvice? 'DefaultInlineAdvice' implies it is an advice provided by the default advisor which no longer is true.

Besides the inlineAdvice contains a reference to the inlineAdvisor, so where the advice comes from is discoverable.

@mtrofin

mtrofin added a comment.EditedJan 8 2021, 7:35 PM

Should we get rid of DefaultInlineAdvice and fold it into the base class InlineAdvice? 'DefaultInlineAdvice' implies it is an advice provided by the default advisor which no longer is true.

Besides the inlineAdvice contains a reference to the inlineAdvisor, so where the advice comes from is discoverable.

@mtrofin

I can look into it, we need to look at its uses on the ML side. Perhaps the logging part can be moved, because the InlineCost stuff is very much default policy-specific.

The modifications to the DefaultInlineAdvice is an attempt to solve the following problem:

  1. In the SampleProfile inliner it emits remarks through the "legacy" interface with emitInlinedInto. For replay if advice generates remarks you'll get duplicate remarks but if it doesn't you get a single set (which is what's happening today and correct).
  2. In the CGSCC inliner in newPM it emits remarks through InlineAdvice via recordInlining and nowhere else. For replay if InlineAdvice generates remarks you're good but if it doesn't you get no remarks.

So to solve this issue I have the ReplayAdvisor use DefaultInlineAdvice with modifications to suppress remarks for the SampleProfile inliner.

I can look into it, we need to look at its uses on the ML side. Perhaps the logging part can be moved, because the InlineCost stuff is very much default policy-specific.

This seems like a cleaner approach than my current solution. I'll need to move the EmitRemarks over to the base class which looks to be okay given it can be defaulted to true. Thoughts?

llvm/lib/Analysis/InlineAdvisor.cpp
65

The SampleProfile.cpp inliner emits its own remarks via the emitInlinedInto interface. Without this you would get double remarks when dumping the SampleProfile.cpp inliner which I want to avoid. The rest of the comments suggests there's a better holistic solution so I'll add my thoughts to a below comment.

The modifications to the DefaultInlineAdvice is an attempt to solve the following problem:

  1. In the SampleProfile inliner it emits remarks through the "legacy" interface with emitInlinedInto. For replay if advice generates remarks you'll get duplicate remarks but if it doesn't you get a single set (which is what's happening today and correct).
  2. In the CGSCC inliner in newPM it emits remarks through InlineAdvice via recordInlining and nowhere else. For replay if InlineAdvice generates remarks you're good but if it doesn't you get no remarks.

So to solve this issue I have the ReplayAdvisor use DefaultInlineAdvice with modifications to suppress remarks for the SampleProfile inliner.

I can look into it, we need to look at its uses on the ML side. Perhaps the logging part can be moved, because the InlineCost stuff is very much default policy-specific.

This seems like a cleaner approach than my current solution. I'll need to move the EmitRemarks over to the base class which looks to be okay given it can be defaulted to true. Thoughts?

I wouldn't block this patch on that - we can do the refactoring subsequently.

mtrofin accepted this revision.Jan 11 2021, 5:48 PM

LGTM; but so I understand something better: the ability to replay is somewhat 'best effort' if the compiler changes (or, of course, the compiled code). May be worth adding a comment about that?

This revision is now accepted and ready to land.Jan 11 2021, 5:48 PM

I wouldn't block this patch on that - we can do the refactoring subsequently.

Sounds good.

LGTM; but so I understand something better: the ability to replay is somewhat 'best effort' if the compiler changes (or, of course, the compiled code). May be worth adding a comment about that?

Thanks for signing off! Your take here is correct, this is operating under a best effort approach to replay. In D94334 I note that testing on mysqld the current solution for CGSCC inline replay is ~94% accurate in replaying when comparing remarks base vs. replay.

Added a comment in ReplayInlineAdvisor describing the best effort approach here.

modimo updated this revision to Diff 315982.Jan 11 2021, 7:47 PM

Add comment about best effort approach of replay.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 1:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
wenlei added inline comments.Jan 12 2021, 9:07 PM
llvm/lib/Analysis/InlineAdvisor.cpp
412

nit: any special reason for adding this? doesn't seem consistent with other remarks we have.

modimo added inline comments.Jan 14 2021, 2:54 PM
llvm/lib/Analysis/InlineAdvisor.cpp
412

If you grab the remark outputs via -Rpass=inline you'll get additional suffix information:

inline.cpp:8:12: remark: _Z3foov inlined into main with (cost=0, threshold=375) at callsite main:2:12; [-Rpass=inline]
    return foo();

The semicolon is to separate the remark from any additional output at the end so when replaying we can match the correct callsite. Something like this would be unneeded for yaml replay but for the current implementation it's necessary for correctness.

wenlei added inline comments.Jan 19 2021, 12:59 PM
llvm/lib/Analysis/InlineAdvisor.cpp
412

By correctness, did you mean the fact that you rely on split(";") in parsing, or something else?

This is not a big deal, but if no other remarks end with ;, it would be good to be consistent. Using split(";") for parsing is just one way of implementing it, and IMO could be changed to favor consistency in remarks output.

modimo added inline comments.Jan 21 2021, 3:11 PM
llvm/lib/Analysis/InlineAdvisor.cpp
412

By correctness, did you mean the fact that you rely on split(";") in parsing, or something else?

Yeah, without that we would store the callsite from remarks as main:2:12 [-Rpass=inline] which would not match the actual callsite string main:2:12 that we query the map with which causes replay to never inline.

This is not a big deal, but if no other remarks end with ;, it would be good to be consistent. Using split(";") for parsing is just one way of implementing it, and IMO could be changed to favor consistency in remarks output.

Doing a search query for OptimizationRemarkAnalysis I see vectorizer ORE uses "." for their terminator so switching to that is better consistency. I'll make the change in an upcoming patch.