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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 5 inline comments as done.
mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:33
+/// requested.
+enum class MLMode : int { Invalid, Rel, Dev };
+
----------------
echristo wrote:
> Might be nice to just spell them out?
I.e. 'Release' and 'Development'? I like that. (will send and updated patch once Inliner.cpp refactoring is done, so leaving this and others marked as not 'done')


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:46
+  void recordInlining() {
+    recordInlining(/*CalleeWasDeleted*/ false, /*SiteWasInlined*/ true);
+  }
----------------
echristo wrote:
> Is there any way we could use an enum(s) instead for these calls rather than boolean arguments?
You mean instead of the 3 members that call into this protected member?

so we have it fleshed out:


```
enum class InliningOutcome {
  Failed,
  Succeeded,
  SucceededWithCalleeRemoval
}
```

and then we only have the one virtual, and no protected:


```
virtual void recordInlining(InliningOutcome Outcome)

```
Does that capture it?

I'm fine with whatever we all feel is the more usable API. One thing to consider - looking at how the APIs are used, I'd actually prefer 2 methods: 1 method for "it failed", and 1 for "it succeeded", with a bool saying whether the callee will be removed or not (see Inliner.cpp:1213 and below, in this patch)

wdyt?


================
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:
> 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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042