[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/ML/InliningAdvisor.h:23
+
+enum class MLMode : int { Invalid, Rel, Dev };
+
----------------
add documentation on MLMode.


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:84
+  virtual std::unique_ptr<PendingInliningRecord>
+  getInliningAdvice(CallBase &CB, bool &AlternativeRecommendation,
+                    bool Mandatory) = 0;
----------------
Perhaps rename 'AlternativeRecommendation' to 'MLDecision'? or 'MLRecommendation'?


================
Comment at: llvm/lib/Analysis/InliningAdvisorAnalysis.cpp:33
+  }
+  M.getContext().emitError(
+      "Could not setup Inlining Advisor for the requested mode and/or options");
----------------
Can this happen, given the option parser filters out other values?


================
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"),
----------------
Invalid mode --> Null mode?


================
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;
----------------
why not use it to handle Entry too? If null, does nothing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042