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

[Eric Christopher via llvm-commits]

On Thu, Apr 30, 2020 at 1:26 PM Mircea Trofin via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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')
>
>
>
Sweet.


> ================
> 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?
>
>
Sounds great. Splitting into multiple methods is a fine solution. You could
split it into 4 if you need or the two with an enum saying whether or not
to remove a callee sounds great. Mostly trying a crusade to avoid adding
new boolean parameters if I can avoid it :)


>
> ================
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200430/fda2948b/attachment.html>