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

[Google Contributors to LLVM via Phabricator via llvm-commits]

google-llvm-upstream-contributions marked 4 inline comments as done.
google-llvm-upstream-contributions added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:220
+static cl::opt<MLMode> EnableMLInliner(
+    "enable-ml-inliner", cl::init(MLMode::Invalid), cl::Hidden,
+    cl::desc("Enable ML policy for inliner. Currently trained for -Oz only"),
----------------
davidxl wrote:
> mtrofin wrote:
> > davidxl wrote:
> > > Invalid mode --> Null mode?
> > How about 'NoML' or 'None'?
> NoML or None or just No sound fine.
Replaced with 'Default', as we always use an InliningAdvisor.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:771
+  if (EnableMLInliner != MLMode::Invalid)
+    MPM.addPass(InliningAdvisorCleanupPass());
   return MPM;
----------------
echristo wrote:
> Can we get a better description of what the cleanup pass is supposed to do? We don't typically have such things and it seems worth calling out in a bit more detail.
Removed the cleanup pass altogether.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1082
 
-      Optional<InlineCost> OIC = shouldInline(*CS, GetInlineCost, ORE);
-      // Check whether we want to inline this callsite.
-      if (!OIC.hasValue()) {
-        setInlineRemark(*CS, "deferred");
+      auto TrivialDecision = llvm::getAttributeBasedInliningDecision(
+          *CS, CS->getCalledFunction(), FAM.getResult<TargetIRAnalysis>(Callee),
----------------
davidxl wrote:
> This should be better computed under 
> 
> if (Advisor) {
> 
>      TrivialDecision = ...
>      if (noinline)
>         continue;
>       Mandatory = ...
>    }
Simplified control flow. The reasoning around mandatory and trivial case will go in the ML-specific implementation.


================
Comment at: llvm/test/Transforms/Inline/inlining-advisor-default.ll:9
+
+; CHECK: Could not setup Inlining Advisor for the requested mode and/or options
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042