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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 2 inline comments as not done.
mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:28
+/// allows recording features/decisions/partial reward data sets.
+class PendingInliningRecord {
+public:
----------------
davidxl wrote:
> No other interfaces in the class?
Not sure I follow. Maybe I should place InliningAdvisor first in the file, followed by PendingInliningRecord (forward - declaring the latter - the current order is just to avoid that fwd decl)?


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:33
+
+  virtual void recordInlining(bool CalleeWasDeleted, bool SiteWasInlined) = 0;
+  virtual ~PendingInliningRecord() = default;
----------------
davidxl wrote:
> two bools are confusing. Perhaps split them? SetWasDeleted, and SetWasInlined?
> 
> Also document this interface.
The idea was to atomically communicate the conclusion of the inlining operation.

I think the liability of setWasDeleted and setWasInlined would be that:
- we'd need a 'finish()' method to say we're done, which adds complexity and more statefulness to the design (harder to track);  or
- just rely on the dtor to do that, but that doesn't address the statefulness
- need to rely on user to remember to call both setWas{Deleted|Inlined}

I realize the current design requires a reader look up and see what the 2 params meant. I think that's a 'lesser evil' than those outlined above.

How about either:
- I decorate the call sites for recordInlining with comments indicating what the parameters mean, or
- add 3 public APIs, as follows (new API -> current API)

recordInlining() -> recordInlining(/*CalleeWasDeleted*/false,  /*SiteWasInlined*/ true)
recordInliningWithCalleeDeleted() -> recordInlining(/*CalleeWasDeleted*/true,  /*SiteWasInlined*/ true)
recordUnsuccessfulInlining() -> recordInlining(/*CalleeWasDeleted*/false,  /*SiteWasInlined*/ false)

wdyt?


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:42
+/// context (e.g. module) and interfaces with a ML model executor (normally AOT,
+/// may optionally be JIT or interpreted, if experimentation is desided) to
+/// evaluate whether inlining should take place.
----------------
davidxl wrote:
> desided --> decided
thanks - I meant 'desired' :)


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:68
+/// The InliningAdvisor needs to capture state right before inlining commences
+/// over a module.
+class InliningAdvisorAnalysis
----------------
davidxl wrote:
> This comments belongs somewhere else -- perhaps in implementation ?
Reworded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042