Annotate inline remarks with two dimensional information
- the link stage, or no LTO.
- the context / driver (CGSCC, ModuleInliner, etc).
Implementation-wise, the annotation is flag gated; flag is turned off by default.
Paths
| Differential D125495
[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
Implementation-wise, the annotation is flag gated; flag is turned off by default.
Diff Detail
Event Timelinemingmingl marked 2 inline comments as done. Comment ActionsResolve the comments. Plus, use llvm_unreachable for getAnnotatedPassName helper function since switch statement has default and should return Comment Actions
add the motivation in Summary. Comment Actions Fix a build error for llvm-project/llvm/lib/Transforms/IPO/SampleProfile.cpp:613:1: error: expected ‘;’ before ‘}’ token Comment Actions It might be worth annotating the early inliner for PGO as well.
Comment Actions 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. Comment Actions
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)
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. To share some thoughts as context, the current implementation is simple and annotates prelink/postlink information for scc-inliner and sample-loader inliner
[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 Actions
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. Comment Actions
Early-inline is a separate phase used in FDO only (both profile gen and profile-use). Yes, they are not using profile data.
Comment Actions
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.
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 marked an inline comment as done. Comment ActionsAdd struct InlineAdvisorParams.
Comment Actions 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:
Comment Actions 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 :-) Comment Actions 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 Comment Actions 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.
Comment Actions
Comment Actions LGTM with a minor comment fix. Thanks!
This revision is now accepted and ready to land.Jun 24 2022, 12:59 AM Comment Actions
Done. Also rephrase it to reflect the updated meaning (from LTO to {LTO, pass}). Thanks! This revision was landed with ongoing or failed builds.Jun 24 2022, 10:07 AM Closed by commit rGe0d069598bc8: [Inline] Annotate inline pass name with link phase information for analysis. (authored by mingmingl). · Explain Why This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 439811 llvm/include/llvm/Analysis/InlineAdvisor.h
llvm/include/llvm/Analysis/ReplayInlineAdvisor.h
llvm/include/llvm/Passes/PassBuilder.h
llvm/include/llvm/Transforms/IPO/Inliner.h
llvm/include/llvm/Transforms/IPO/ModuleInliner.h
llvm/lib/Analysis/InlineAdvisor.cpp
llvm/lib/Analysis/ReplayInlineAdvisor.cpp
llvm/lib/Passes/PassBuilderPipelines.cpp
llvm/lib/Passes/PassRegistry.def
llvm/lib/Transforms/IPO/Inliner.cpp
llvm/lib/Transforms/IPO/ModuleInliner.cpp
llvm/lib/Transforms/IPO/SampleProfile.cpp
llvm/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll
llvm/test/LTO/Resolution/X86/diagnostic-handler-remarks.ll
llvm/test/LTO/X86/diagnostic-handler-remarks-with-hotness.ll
llvm/test/LTO/X86/diagnostic-handler-remarks.ll
llvm/test/ThinLTO/X86/diagnostic-handler-remarks-with-hotness.ll
llvm/test/ThinLTO/X86/diagnostic-handler-remarks.ll
llvm/test/Transforms/Inline/optimization-remarks-passed-yaml.ll
|
Is it possible to define sample loader inliner here too ?