[PATCH] D79042: [llvm] Add interface to drive inlining decision using ML model

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:50
+/// obligations.
+class PendingInliningRecord {
+public:
----------------
To make the naming more consistent, suggest changing the name to

 InlineAdvise  or InlineRecord


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:111
+/// Interface for deciding whether to inline a call site or not.
+class InliningAdvisor {
+public:
----------------
InliningAdviser --> InlineAdviser


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:121
+  virtual std::unique_ptr<PendingInliningRecord>
+  getInliningAdvice(CallBase &CB, FunctionAnalysisManager &FAM) = 0;
+
----------------
perhaps shorten it : getAdvice


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:158
+/// reusable as-is for inliner pass test scenarios, as well as for regular use.
+class DefaultInliningAdvisor : public InliningAdvisor {
+public:
----------------
DefaultInliningAdvisor --> DefaultInlineAdvisor


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:172
+/// needs to capture state right before inlining commences over a module.
+class InliningAdvisorAnalysis
+    : public AnalysisInfoMixin<InliningAdvisorAnalysis> {
----------------
->InlineAdviserAnalysis


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:51
+namespace {
+class DefaultPendingRecord : public PendingInliningRecord {
+public:
----------------
DefaultInlineRecord


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:628
     IP.HintThreshold = 325;
-
-    CGSCCPassManager CGPipeline(DebugLogging);
-
-    CGPipeline.addPass(InlinerPass(IP));
+    ModuleInlinerWrapperPass MIWP(IP, DebugLogging);
+    CGSCCPassManager &CGPipeline = MIWP.getPM();
----------------
This is for InstrPGO early inlining. I don't see the need to hook up advisor here -- even there is need in the future, it may need a different model. recommend leave this alone and add some comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1074
+  // module pipeline by walking the SCCs in postorder (or bottom-up).
+  if (MaxDevirtIterations > 0)
+    MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(
----------------
Is this guard needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042