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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin 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.
+///
----------------
davidxl wrote:
> perhaps use compile time instead of runtime
reworded, I think it's more clear now.


================
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);
----------------
davidxl wrote:
> should this refactoring be done seperately?
Yes, if we're OK with this direction, I'll add first the InliningAdvisor.{h|cpp} with just these functions.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:730
 
+  if (EnableMLInliner != MLMode::Invalid) {
+    MPM.addPass(RequireAnalysisPass<InliningAdvisorAnalysis, Module>());
----------------
echristo wrote:
> Extra braces.
This probably got out of sync with the source, or got addressed by the latest patch.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:901
+  // an advisor, ensure it is always informed when we're done with a scc.
+  class AdvisorExitCapture final {
+    InliningAdvisor *const Advisor;
----------------
davidxl wrote:
> why not use it to handle Entry too? If null, does nothing.
Un-did the entry handling, because we're using the scope_exit API.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:679
+
+  Optional<DefaultInliningAdvisor> OwnedAdvisor;
+  InliningAdvisor *Advisor = nullptr;
----------------
davidxl wrote:
> davidxl wrote:
> > Put the advisor computation code into its own helper method?
> OwnedAdvisor --> DefaultAdvisor or NullAdvisor or NopAdvisor?
It's not a Null/Nop advisor, it's today's behavior. Also, you can get a "DefaultAdvisor" through the analysis pass. The key difference here is lifetime management: when InlinerPass is used stand-alone as a SCC pass, we make and own the DefaultInliningAdvisor instance.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:901-911
+  class AdvisorExitCapture final {
+    InliningAdvisor *const Advisor;
+
+  public:
+    AdvisorExitCapture(InliningAdvisor *A) : Advisor(A) {}
+    ~AdvisorExitCapture() {
+      if (Advisor)
----------------
dblaikie wrote:
> I'm surprised we don't have a scope-guard like device in LLVM already, but one other way to address this (perhaps that's what everyone else has done in LLVM so far) would be to pull the body of the function out into a separate function, so you can easily ensure the end operation is done.
> 
> Otherwise, if you prefer to keep it this way it seems like a bit of overkill to make it a class - probably a struct, used {} init rather than a ctor, etc to keep it simple, given it's only used once/immediately.
There is a scope-guarding API! Thanks for the pointer. Addressed.


================
Comment at: llvm/test/Transforms/Inline/inlining-advisor-default.ll:9
+
+; CHECK: Could not setup Inlining Advisor for the requested mode and/or options
----------------
google-llvm-upstream-contributions wrote:
> davidxl wrote:
> > mtrofin wrote:
> > > davidxl wrote:
> > > > I wonder if it is useful to have a mock mode for testing purpose where the ML code is barebone but not fully statically compiled out.
> > > If we only use that for test, then the 'dev' mode where we just capture logs does exactly that - and I plan on getting a build bot setup for this; and it has a test, of course (and doesn't add complexity)
> > it might be better to exercise some tests by default (without relying on bot). When there is a breakage, it is also easier for folks to reproduce.
> Done. We always use an InliningAdvisor. Also added tests side-by-side the inliner tests that were using the new pass manager, to test the ModuleInlinerPassWrapper. So the control flow in the inliner is tested in all cases, and these 'duplicate' cases test the wrapper and the logic getting the advisor from it.
Done, as we always use an InliningAdvisor. All the bots will test is ML-specific implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042