[PATCH] D79042: [llvm] Add interface to drive inlining decision using ML model
Mircea Trofin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat May 2 17:29:43 PDT 2020
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
More information about the llvm-commits
mailing list