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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl marked 7 inline comments as done.
mingmingl added a comment.

This patch is not ready for review (D126833 <https://reviews.llvm.org/D126833> is the parent revision, and I'll need to merge D126824 <https://reviews.llvm.org/D126824>)

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

thanks for reviews and comments!



================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:43
 
+enum class InlineAdvisorContext : int {
+  EarlyInliner,
----------------
davidxl wrote:
> Is it possible to define sample loader inliner here too ?
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.



================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:198
+  ///    For example, a inline decision in postlink CGSCC pass could be a result
+  ///    of applying heuristics, but could be a result of mandatory inline as
+  ///    well.
----------------
davidxl wrote:
> If we split out AlwaysInline, then it will be less ambiguous.
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


================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:220
+  buildModuleOptimizationPipeline(OptimizationLevel Level,
+                                  ThinOrFullLTOPhase LTOPhase);
 
----------------
davidxl wrote:
> This change can be split out into an independent patch.
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)


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:568
+    return "replay-sample-profile-inline";
+  case (InlineAdvisorContext::UnspecifiedInlinerContext):
+    return DEBUG_TYPE;
----------------
davidxl wrote:
> In what case it is unspecified?
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125495