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

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

In D125495#3516513 <https://reviews.llvm.org/D125495#3516513>, @kazu wrote:

> In D125495#3510338 <https://reviews.llvm.org/D125495#3510338>, @mingmingl wrote:
>
>> In D125495#3510137 <https://reviews.llvm.org/D125495#3510137>, @davidxl wrote:
>>
>>> 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.



================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:60
+static cl::opt<bool>
+    AnnotateInlineLTOPhase("annotate-inline-lto-phase", cl::Hidden,
+                           cl::init(false),
----------------
mingmingl wrote:
> 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)
Probably changing the option name to be 'annotate-inline-phase'.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:109
+  case llvm::ThinOrFullLTOPhase::ThinLTOPreLink:
+    return "thin-lto-prelink-default-inline";
+  case llvm::ThinOrFullLTOPhase::FullLTOPreLink:
----------------
mingmingl wrote:
> 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
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.

 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125495