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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:30-31
+public:
+  PendingInliningRecord(PendingInliningRecord &&) = delete;
+  PendingInliningRecord(const PendingInliningRecord &) = delete;
+
----------------
You can leave these off if you like - they're implied by the existence of a user-declared destructor. (that'll automatically delete/remove the copy/move operations)


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:36-37
+
+protected:
+  PendingInliningRecord() = default;
+};
----------------
This is the default (well, it'd be public by default, but it can't be called directly because of the presence of a pure virtual function in the class) so you could omit it.


================
Comment at: llvm/lib/Analysis/InliningAdvisorAnalysis.cpp:25
+                     "pass manager");
+    return nullptr;
+  case MLMode::Dev:
----------------
Remove dead code after an unreachable here.


================
Comment at: llvm/lib/Analysis/InliningAdvisorAnalysis.cpp:28-35
+#if LLVM_HAVE_TF_API || LLVM_HAVE_TF_AOT
+    return InliningAdvisor::create(M, MAM, Mode);
+#endif
+    break;
+  }
+  M.getContext().emitError(
+      "Could not setup Inlining Advisor for the requested mode and/or options");
----------------
I'd probably picture this not using the #ifdef stuff here - InliningAdvisor.h has to be unconditionally compiled anyway (since it provides the IniliningAdvisor base class)... oh - but you weren't planning on unconditionally compiling a .cpp file that implements it? That would be good to avoid - it'd be unfortunate to have a header we have to include with silent contracts about which parts of the header you can use (& we shouldn't be in a position where certain member functions has to be defined in a header or must be defined in the .cc file or there will be subtle problems - that'd be surprising/confusing for maintenance). I'd probably go further and say maybe the whole header should live outside the ML directory? And either it'd provide the InliningAdvisor::create function, and use #ifdefs in its own implementation (seems fair - it's already going to need to test TF_API and TF_AOT to decide which kind of implementations it has available) so it can be called unconditionally - and just returns nullptr if neither API is available.

That could be implemented in llvm/Analysis (non-ML), InliningAdvisorFactory, just a header with one function declared in it, and a single source file implementing it that's just "ifdef TF_API return new TFAPIThing #elseif TF_AOT ... etc")


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:901-911
+  class AdvisorExitCapture final {
+    InliningAdvisor *const Advisor;
+
+  public:
+    AdvisorExitCapture(InliningAdvisor *A) : Advisor(A) {}
+    ~AdvisorExitCapture() {
+      if (Advisor)
----------------
I'm surprised we don't have a scope-guard like device in LLVM already, but one other way to address this (perhaps that's what everyone else has done in LLVM so far) would be to pull the body of the function out into a separate function, so you can easily ensure the end operation is done.

Otherwise, if you prefer to keep it this way it seems like a bit of overkill to make it a class - probably a struct, used {} init rather than a ctor, etc to keep it simple, given it's only used once/immediately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042