[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:
----------------
No other interfaces in the class?


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:33
+
+  virtual void recordInlining(bool CalleeWasDeleted, bool SiteWasInlined) = 0;
+  virtual ~PendingInliningRecord() = default;
----------------
two bools are confusing. Perhaps split them? SetWasDeleted, and SetWasInlined?

Also document this interface.


================
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.
----------------
desided --> decided


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:50
+  virtual std::unique_ptr<PendingInliningRecord>
+  shouldInline(CallBase *CB, bool &AlternativeRecommendation, bool Mandatory,
+               int CostEstimate) = 0;
----------------
nit: shouldInline --> getInlineAdvise?


================
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
----------------
This comments belongs somewhere else -- perhaps in implementation ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042