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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:28
+/// allows recording features/decisions/partial reward data sets.
+class PendingInliningRecord {
+public:
----------------
mtrofin wrote:
> 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)?
what I meant is that this class defined here has no other accessor interfaces (other than one setter).


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:33
+
+  virtual void recordInlining(bool CalleeWasDeleted, bool SiteWasInlined) = 0;
+  virtual ~PendingInliningRecord() = default;
----------------
mtrofin wrote:
> 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?
Making recordInlining private and add wrappers to them sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042