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

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

This is pretty cool. I've got one high level question and then a few minor things in the patch.

High level: Would it be better to split this out even more rather than the if/elses in the Inliner? i.e. rather than conditionalizing it just have N paths through. It may mean a little bit more duplicated or factored code, but would probably be a little easier to read through.

Thoughts?

-eric



================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:33
+/// requested.
+enum class MLMode : int { Invalid, Rel, Dev };
+
----------------
Might be nice to just spell them out?


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:46
+  void recordInlining() {
+    recordInlining(/*CalleeWasDeleted*/ false, /*SiteWasInlined*/ true);
+  }
----------------
Is there any way we could use an enum(s) instead for these calls rather than boolean arguments?


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:730
 
+  if (EnableMLInliner != MLMode::Invalid) {
+    MPM.addPass(RequireAnalysisPass<InliningAdvisorAnalysis, Module>());
----------------
Extra braces.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:771
+  if (EnableMLInliner != MLMode::Invalid)
+    MPM.addPass(InliningAdvisorCleanupPass());
   return MPM;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042