[PATCH] D125495: [Inline][Remark] Annotate inline pass name with link phase information for analysis.

Mingming Liu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu May 12 16:49:25 PDT 2022


mingmingl marked an inline comment as done.
mingmingl added a comment.

In D125495#3510137 <https://reviews.llvm.org/D125495#3510137>, @davidxl wrote:

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

If I understand correctly, this aims to annotate the early inline transformations [1] are made without PGO profiles.

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

In D125495#3510215 <https://reviews.llvm.org/D125495#3510215>, @wenlei wrote:

> 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



================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:60
+static cl::opt<bool>
+    AnnotateInlineLTOPhase("annotate-inline-lto-phase", cl::Hidden,
+                           cl::init(false),
----------------
davidxl wrote:
> document possible inline pass names here.
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)


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:109
+  case llvm::ThinOrFullLTOPhase::ThinLTOPreLink:
+    return "thin-lto-prelink-default-inline";
+  case llvm::ThinOrFullLTOPhase::FullLTOPreLink:
----------------
davidxl wrote:
> why adding 'default' in the name?
'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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:600
+
+  switch (phase) {
+  case llvm::ThinOrFullLTOPhase::ThinLTOPreLink:
----------------
davidxl wrote:
> I think it is better to also add 'sample-loader' into the pass string as well.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125495



More information about the llvm-commits mailing list