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

[David Blaikie via Phabricator via llvm-commits]

dblaikie marked an inline comment as done.
dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:30-31
+public:
+  PendingInliningRecord(PendingInliningRecord &&) = delete;
+  PendingInliningRecord(const PendingInliningRecord &) = delete;
+
----------------
dblaikie wrote:
> 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)
Yeah, looks like this comment ended up out-of-place, was writing it based on a previous version of the code. (the email had the right quoted text, FWIW)

& in any case, my comment was incorrect - the user-declared dtor doesn't disable copy/move, so makes sense to leave them.


================
Comment at: llvm/include/llvm/Analysis/ML/InliningAdvisor.h:36-37
+
+protected:
+  PendingInliningRecord() = default;
+};
----------------
dblaikie wrote:
> 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.
This comment was about the protected defaulted default ctor - well, you still need to define it (because of the deleted copy/move ctors) but you could define it as public as much as as protected - either way no one can instantiate this class because it has pure virtual functions. Not a huge deal either way I guess.


================
Comment at: llvm/lib/Analysis/InliningAdvisorAnalysis.cpp:25
+                     "pass manager");
+    return nullptr;
+  case MLMode::Dev:
----------------
dblaikie wrote:
> Remove dead code after an unreachable here.
(the "return nullptr;" after "llvm_unreachable" is dead - llvm_unreachable is trap/abort/UB-in-optimized builds, the return is meaningless/would be DCE'd anyway, best to remove it)


================
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");
----------------
mtrofin wrote:
> dblaikie wrote:
> > 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")
> I don't follow. The ifdef is in the InliningAdvisorAnalysis.cpp, which is unconditionally compiled. 
Ah, sorry, that was a bit rambly.

What I mean is, why is the call to InliningAdvisor::create conditionalized? I assume it's because, while the declaration of InliningAdvisor::create is always available in InliningAdvisor.h, its implementation is not (because, I assume, everything in Analysis/ML is conditionally compiled only if TF is available?). I think it's problematic to have a header that only works because certain parts of it must remain in the header & can't be moved to an implementation file (because there is no unconditionally compiled implementation file to move them to).

Hmm, now I'm confused - the header has parts defined in lib/Analysis (like InliningAdvisorAnalysis::* here) and parts defined in lib/Analysis/MC (like InliningAdvisor::create - well, that isn't defined anywhere right now, it seems - but I assume the idea was to implement it in lib/Analysis/MC?)).

I think it might be tidier to have the lib/Analysis/MC entry point be very small - just the InliningAdvisorAnalysis::create - and if the goal is to have everything in lib/Analysis/MC be conditionally compiled using CMake and the presence of TF, then maybe having a header in lib/Analysis that declares (& defines inline, or out-of-line in its own .cpp file in lib/Analysis, either way) and uses macros to define the implementation as either a no-op (because it can't call into lib/Analysis/MC because it doesn't exist) or as the functional implementation that instantiates the desired class in lib/Analysis/MC).

I think having the factory function always be available, but it either is implemented as a no-op (returning a null unique_ptr) when the library isn't available keeps the boundary in a very clear/small part of the code, not amongst any other code (not that InliningAdvisorAnalysis.cpp is especially long by any means).

I wouldn't put any of the #ifdefs in if they're always false when this patch is committed - that's dead/untested code, which is best not to commit (a comment describing what else might go there is OK though). 

I think there's some discussion of having a stub implementation for testing? I think that'd be appropriate to go in with this patch - otherwise there's a lot of dead code (not just these #ifdefs, but all the InliningAdvisor results, etc aren't tested because there's never a non-null implementation in this patch). (I'm not sure how that stub would be powered - either a hardcoded general implementation (does something silly like forcibly inline every second function) with DEBUG outputs to indicate how it's being called - or perhaps reading from an external file to dictate what paths to go down... or unit tested, but that's not usually how optimizations are tested (maybe there's a reasonable amount of unit testing for analyses though? I'm not sure - I haven't looked))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042