[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/InliningAdvisor.h:32
+/// In this mode, we compile ahead of time a pre-trained ML model. This mode has
+/// low runtime overhead, and no additional runtime dependencies.
+///
----------------
perhaps use compile time instead of runtime


================
Comment at: llvm/include/llvm/Analysis/InliningAdvisor.h:204
+/// caller if \p CB is suppressed for inlining.
+bool shouldBeDeferred(Function *Caller, InlineCost IC, int &TotalSecondaryCost,
+                      function_ref<InlineCost(CallBase &CB)> GetInlineCost);
----------------
should this refactoring be done seperately?


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:679
+
+  Optional<DefaultInliningAdvisor> OwnedAdvisor;
+  InliningAdvisor *Advisor = nullptr;
----------------
Put the advisor computation code into its own helper method?


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:679
+
+  Optional<DefaultInliningAdvisor> OwnedAdvisor;
+  InliningAdvisor *Advisor = nullptr;
----------------
davidxl wrote:
> Put the advisor computation code into its own helper method?
OwnedAdvisor --> DefaultAdvisor or NullAdvisor or NopAdvisor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042