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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 2 inline comments 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:
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042