<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Apr 30, 2020 at 1:26 PM Mircea Trofin via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">mtrofin marked 5 inline comments as done.<br>
mtrofin added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:33<br>
+/// requested.<br>
+enum class MLMode : int { Invalid, Rel, Dev };<br>
+<br>
----------------<br>
echristo wrote:<br>
> Might be nice to just spell them out?<br>
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')<br>
<br>
<br></blockquote><div><br></div><div>Sweet.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
================<br>
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:46<br>
+  void recordInlining() {<br>
+    recordInlining(/*CalleeWasDeleted*/ false, /*SiteWasInlined*/ true);<br>
+  }<br>
----------------<br>
echristo wrote:<br>
> Is there any way we could use an enum(s) instead for these calls rather than boolean arguments?<br>
You mean instead of the 3 members that call into this protected member?<br>
<br>
so we have it fleshed out:<br>
<br>
<br>
```<br>
enum class InliningOutcome {<br>
  Failed,<br>
  Succeeded,<br>
  SucceededWithCalleeRemoval<br>
}<br>
```<br>
<br>
and then we only have the one virtual, and no protected:<br>
<br>
<br>
```<br>
virtual void recordInlining(InliningOutcome Outcome)<br>
<br>
```<br>
Does that capture it?<br>
<br>
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)<br>
<br>
wdyt?<br>
<br></blockquote><div><br></div><div>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 :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
================<br>
Comment at: llvm/test/Transforms/Inline/inlining-advisor-default.ll:9<br>
+<br>
+; CHECK: Could not setup Inlining Advisor for the requested mode and/or options<br>
----------------<br>
davidxl wrote:<br>
> 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.<br>
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)<br>
<br>
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D79042/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D79042/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D79042" rel="noreferrer" target="_blank">https://reviews.llvm.org/D79042</a><br>
<br>
<br>
<br>
</blockquote></div></div>