Page MenuHomePhabricator

[Inline][Remark] Annotate inline pass name with link phase information for analysis.
ClosedPublic

Authored by mingmingl on May 12 2022, 12:29 PM.

Details

Summary

Annotate inline remarks with two dimensional information

  1. the link stage, or no LTO.
  2. the context / driver (CGSCC, ModuleInliner, etc).

Implementation-wise, the annotation is flag gated; flag is turned off by default.

Diff Detail

Event Timeline

mingmingl created this revision.May 12 2022, 12:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 12:29 PM
mingmingl requested review of this revision.May 12 2022, 12:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 12:29 PM

can you put the motivation in the description?

llvm/lib/Analysis/InlineAdvisor.cpp
104

these parens aren't necessary

llvm/lib/Passes/PassBuilderPipelines.cpp
1137–1138

ternary operator is unnecessary

mingmingl updated this revision to Diff 429070.May 12 2022, 2:18 PM
mingmingl marked 2 inline comments as done.
mingmingl edited the summary of this revision. (Show Details)

Resolve the comments.

Plus, use llvm_unreachable for getAnnotatedPassName helper function since switch statement has default and should return

can you put the motivation in the description?

add the motivation in Summary.

mingmingl updated this revision to Diff 429075.May 12 2022, 2:26 PM

Fix a build error for llvm-project/llvm/lib/Transforms/IPO/SampleProfile.cpp:613:1: error: expected ‘;’ before ‘}’ token

It might be worth annotating the early inliner for PGO as well.

llvm/lib/Analysis/InlineAdvisor.cpp
60

document possible inline pass names here.

109

why adding 'default' in the name?

llvm/lib/Transforms/IPO/SampleProfile.cpp
600

I think it is better to also add 'sample-loader' into the pass string as well.

wenlei added a subscriber: modimo.May 12 2022, 3:33 PM

there's also an always inline pass (AlwaysInliner.cpp).

cc @modimo in case this affects inline replay as well. theoretically replay can be more accurate leveraging accurate pass info.

mingmingl marked an inline comment as done.EditedMay 12 2022, 4:49 PM

It might be worth annotating the early inliner for PGO as well.

To confirm if I'm understanding it correctly, you mean we should annotate the "early" (i.e., not profile-driven) information for those inline transformations that are are made without PGO profiles [1].

Ask since the early inliner is annotated with prelink/postlink information (same line around line 630 by construction of ModuleInlinerWrapperPass instance)

[1] https://github.com/llvm/llvm-project/blob/e91a73de24d60954700d7ac0293c050ab2cbe90b/llvm/lib/Passes/PassBuilderPipelines.cpp#L630

there's also an always inline pass (AlwaysInliner.cpp).

cc @modimo in case this affects inline replay as well. theoretically replay can be more accurate leveraging accurate pass info.

thanks for mentioning this!

Re always-inliner, it's currently providing always-inline information in pass remark name [2]. So do we want to annotate the pass name for always-inline?

Re replay inline, the pass name for replay inline advices will remain the same with -annotate-inline-lto-phase on -> the replay inline advisor class implementation remain the same before and after this change.
For my understanding, is the replay inline advisor used for development (conveniently inline the functions according to an external file)? Is the replay inline advisor used for production build?

To share some thoughts as context, the current implementation is simple and annotates prelink/postlink information for scc-inliner and sample-loader inliner

  • To elaborate, for scc inliner, only when constructors of default-inline-advisor provides the optional LTOPhase and flag is on, the pass name change.
    • From this perspective, if new implementations of InlineAdvisor/InlineAdvice interface are added, they won't pick up the change inadvertently.
  • On the other hand, if we want to carry implementation-specific information for general inline advice (e.g. always, replay, module-scc or scc) , one option is to add new virtual method in InlineAdvisor/InlineAdvice.

[2] https://github.com/llvm/llvm-project/blob/e91a73de24d60954700d7ac0293c050ab2cbe90b/llvm/lib/Transforms/IPO/AlwaysInliner.cpp#L86 and https://github.com/llvm/llvm-project/blob/e91a73de24d60954700d7ac0293c050ab2cbe90b/llvm/lib/Analysis/InlineAdvisor.cpp#L477

llvm/lib/Analysis/InlineAdvisor.cpp
60

Before changing option name (that involves updating tests), shall the option be renamed as 'annotate-scc-inline-lto-phase'.

Regardless, I should probably document the affected inline pass names in description (re always-inline and replay-inline in another comment).

(keep this comment open)

109

'default' was originally added to indicate the particular instance of DefaultInlineAdvice is from DefaultInlineAdvisor (i.e., heuristic-based), and not from ReplayInlineAdvisor [1].

However, I realize this is not necessary now -> ReplayInlineAdvisor won't provide LTOPhase when constructing an advice, so 'default' could be erased. Before making a change (that involves updating tests), shall I call it 'thin-lto-prelink-inline' or 'thin-lto-prelink-scc-inline'?

(keep this comment open)

[1] https://github.com/llvm/llvm-project/blob/b1aed14bfea07508e4b9d864168c1ae6b5b5c665/llvm/lib/Analysis/ReplayInlineAdvisor.cpp#L116

llvm/lib/Transforms/IPO/SampleProfile.cpp
600

The macros has "CSINLINE_DEBUG", which has a substring "sample-profile". So they currently carry the pass information ("sample-profile"). Nevertheless it took me a while to realize it's "sample-profile" (not "sample-loader") when analyzing the remarks.

kazu added a comment.May 16 2022, 10:43 AM

It might be worth annotating the early inliner for PGO as well.

To confirm if I'm understanding it correctly, you mean we should annotate the "early" (i.e., not profile-driven) information for those inline transformations that are are made without PGO profiles [1].

Ask since the early inliner is annotated with prelink/postlink information (same line around line 630 by construction of ModuleInlinerWrapperPass instance)

I am not David, but yes, it would be nice to be able to differentiate the early inlining and rhw profile-driven inlining in the FDO scenario.

It might be worth annotating the early inliner for PGO as well.

To confirm if I'm understanding it correctly, you mean we should annotate the "early" (i.e., not profile-driven) information for those inline transformations that are are made without PGO profiles [1].

Ask since the early inliner is annotated with prelink/postlink information (same line around line 630 by construction of ModuleInlinerWrapperPass instance)

I am not David, but yes, it would be nice to be able to differentiate the early inlining and rhw profile-driven inlining in the FDO scenario.

Early-inline is a separate phase used in FDO only (both profile gen and profile-use). Yes, they are not using profile data.

llvm/lib/Analysis/InlineAdvisor.cpp
60

Probably changing the option name to be 'annotate-inline-phase'.

109

There are two dimensions information for annotation: one is the phase, and the other is the inline driver.

For inline phase we have:

Early (FDO only), SampleLoader (AFDO, non thinLTO), SampleLoaderPrelink(AFDO, thinLTO), SampleLoaderPostlink (AFDO, thinLTO), mainInline (main inline phase, non ThinLTO), preLinkInl (thinLTO), postLinkInl(ThinLTO), always ...

For inliner flavors, we have:

BU (bottom up SCC inliner), TD (towndown SCC inliner -- used by SamplerLoaderOnly), ML (ML based inliner), and PriorityBased (module inliner).

To combine them together, we can have something like:

early-BU
sample-TD
sample-prelink-TD
sample-postlink-TD
prelink-BU
postlink-BU
main-BU
main-ML
main-priority
prelink-priority

etc.

Re replay inline, the pass name for replay inline advices will remain the same with -annotate-inline-lto-phase on -> the replay inline advisor class implementation remain the same before and after this change.
For my understanding, is the replay inline advisor used for development (conveniently inline the functions according to an external file)? Is the replay inline advisor used for production build?

I've been only using it for development as a way to stabilize inlining when comparing compiler changes that impact function size. That way, we can better examine the impact of the change rather than the resulting inlining differences. It can certainly be used in production but I don't know of a user doing so.

there's also an always inline pass (AlwaysInliner.cpp).

cc @modimo in case this affects inline replay as well. theoretically replay can be more accurate leveraging accurate pass info.

Replay currently is tied to an entire pass, so if replay is specified for the CGSCC inliner all invocations will perform replay. Importantly, there's no distinction between which instance of the CGSCC inliner the replay remarks came from or uses--I've been manually controlling post-LTO/pre-LTO by changing where the replay/remark flag is passed. With lto phase and inliner name tagging on the remarks generated, replay can now know which exact pass to apply them on and theoretically generate better results.

mingmingl updated this revision to Diff 433118.Tue, May 31, 9:51 AM
mingmingl marked an inline comment as done.
mingmingl edited the summary of this revision. (Show Details)

Add struct InlineAdvisorParams.

  • Ctor of inliner advisor provides this struct with two dimensional information.
  • Inline advice instances query this struct to get the annotation information.
mingmingl marked an inline comment as done.Tue, May 31, 10:08 AM

The patch annotates SampleProfile inline, as well as the inline passes that talks to DefaultInlineAdvisor to get an advice (CGSCC, ModuleInline, ReplayInline).

Some rationale regarding why not all inline related passes are annotated:

  1. The inline advisor interface exposes ORE instance (OptimizationRemarkEmitter) and four interfaces (like recordInliningImpl) to derived classes (protected methods and fields); so derived classes can decide what messages to emit and the pass name to use. From this perspective, non-trivial refactors and changes to existing interface class are necessary to "revoke" the derived classes' access to ORE instance.
  1. Relatedly, AlwaysInliner and MLInliner use ORE instance without talking to InlineAdvisor interface (similar to sample-profile pass), which is actually much simpler to modify (that could be done in separate patches)

Just fwiw, the sample-profile annotation and default-inline-advisor annotation are orthogonal and gated by two different options. I'm willing to move either one into another patch if a smaller patch is easier to review :-)

resolve the following compile failure and run ninja check-all to verify

external/llvm-project/llvm/lib/Analysis/InlineAdvisor.cpp:562:31: error: no member named 'AlwaysInliner' in 'llvm::InlineAdvisorContext'; did you mean 'EarlyInliner'?
  case (InlineAdvisorContext::AlwaysInliner):
        ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
                              EarlyInliner

I suggest splitting out the LTOPhase passing interface changes into a different patch. That one is more mechanical. After which this patch can be greatly simplified.

llvm/include/llvm/Analysis/InlineAdvisor.h
44

Is it possible to define sample loader inliner here too ?

45

AlwaysInline should probably be defined here too.

203

a inline --> an inline

204

If we split out AlwaysInline, then it will be less ambiguous.

llvm/include/llvm/Passes/PassBuilder.h
220

This change can be split out into an independent patch.

llvm/lib/Analysis/InlineAdvisor.cpp
568

In what case it is unspecified?

mingmingl marked 7 inline comments as done.Thu, Jun 2, 2:18 PM

This patch is not ready for review (D126833 is the parent revision, and I'll need to merge D126824)

Reply the previous comments to continue the discussion before revising this patch.

thanks for reviews and comments!

llvm/include/llvm/Analysis/InlineAdvisor.h
44

Done in D126833.

p.s. originally thought sample profile inliner owns an ORE (OptimizationRemarkEmitter) to emit remarks (i.e., without using InlineAdvisor or InlineAdvice). Now make Context class general so all inline passes could provide this context information.

204

If I understand correctly, you mean the MandatoryInlineAdvice (given out by CGSCC inliner pass) [1] should be annotated with always-inline (which is indeed much clearer)

[1] https://github.com/llvm/llvm-project/blob/2dfe41944658a006dc3f89316b1448cfec0131c6/llvm/lib/Analysis/InlineAdvisor.cpp#L612-L617

llvm/include/llvm/Passes/PassBuilder.h
220

To confirm my understanding, is this independent patch supposed to provide InlineAdvisorParams when building the pipeline?

If yes, it sounds like this patch could be reduced to change derived classes (e.g., DefaultInlineAdvisor`) to make use of Optional<InlineAdvisorParam>, but still no-op since pipeline provides NoneType::None.

(keep this comment open)

llvm/lib/Analysis/InlineAdvisor.cpp
109

Introduced struct InlineAdvisorParams so it could convey dimensional information to InlineAdvisor.

As shown in regression tests, the current annotation looks similar to the examples, yet without 1) or 2) for simplicity.

  1. TD/BU information is not added.
  2. priority information is not added.

Presumably, compiler users could deduce TD/BU from the pass name (sample-profile, CGSCC), and priority information from the compiler options (that they specify on their own).

568

On a second thought UnspecifiedInlinerContext shouldn't exist since InlineAdvisorParams is of type Optional.

In other words, if context isn't specified, InlineAdvisorParams should be NoneType::None.

Removed this entry.

mingmingl updated this revision to Diff 439420.Thu, Jun 23, 8:44 AM
mingmingl marked 4 inline comments as done.
mingmingl edited the summary of this revision. (Show Details)

Sync base to head.

kazu accepted this revision.Fri, Jun 24, 12:59 AM

LGTM with a minor comment fix. Thanks!

llvm/lib/Analysis/InlineAdvisor.cpp
63

Insert 'and' like ModuleInliner, and ReplayInliner.

This revision is now accepted and ready to land.Fri, Jun 24, 12:59 AM
mingmingl updated this revision to Diff 439801.Fri, Jun 24, 9:32 AM
mingmingl edited the summary of this revision. (Show Details)

Update the description of option --annotate-inline-phase.

LGTM with a minor comment fix. Thanks!

Done. Also rephrase it to reflect the updated meaning (from LTO to {LTO, pass}). Thanks!

This revision was landed with ongoing or failed builds.Fri, Jun 24, 10:07 AM
This revision was automatically updated to reflect the committed changes.