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

Sat May 2 21:12:09 PDT 2020

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

================
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");
----------------
dblaikie wrote:
> 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))
Initially, I was planning to implement InliningAdvisor::create in lib/Analysis/ML/InliningAdvisor.cpp. To summarize your proposal, also factoring in the stub implementation proposal:

- declare a "llvm::createInliningAdvisor" in include/Analysis/ML/InliningAdvisor.h, implement it in lib/Analysis/ML/InliningAdvisor.cpp (instead of the static InliningAdvisor::create)
- in lib/Analysis/InliningAdvisorAnalysis.cpp, include include/Analysis/ML/InliningAdvisor.h conditionally
- add the conditional include and the conditional call to llvm::createInliningAdvisor in the patch following this one, when we add the ML stuff

- for the stub implementation, I see 2 options, with all the above staying the same :
  1) there'll be a 4th MLMode, 'Test' or something
     - the implementation of InliningAdvisor will be a no-op here, in InliningAdvisorAnalysis.cpp
     - the case statement will just new-up that no-op directly (i.e. it won't be going through the llvm::createInliningAdvisor when it gets introduced)
     - the noop would be mimic the codepath for 'Development' mode, when we want to learn starting from the current behavior, and parrot back the cost-based result (i.e. whether shouldInline in Inliner.cpp says "yes" or "no"). That still means we don't test the 'Release' code path in Inliner.cpp, which skips calling shouldInline. 

   2) no 4th mode for MLMode, instead: 
      - we always have an InliningAdvisor, i.e. even without ML in the picture
      - the default InliningAdvisor performs today's shouldInline
      - Inliner.cpp's code gets simplified - no conditional 'if (Advisor)' or anything. All the InlineCost-based reporting happens in the default InliningAdvisor's corresponding PendingInliningRecord implementation.
      - the Development mode behavior of calling today's decision making process is encapsulated in it, and thus tested there

I'd pitch for option 2 - simpler code, I think

What do folks think? (Probably it'd help the discussion if I made the update to the patch)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042