[InlineAdvisor] Allow loading advisors as plugins

[IBricchi]

Summary

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

The PluginInlineAdvisorAnalysis class serves as the entry point for dynamic advisors. Plugins must register instances of this class to provide their own InliningAdvisor.

[mtrofin]

lgtm, but could you please get someone to look at the cmakelists stuff, I'm not sure if that's the most canonical llvm way to do that... maybe @phosek, but AFAIK he may be out for the remainder of the year

[IBricchi]

We will see if @phosek has time, but for context we imitated https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt for the cmake changes.

[mtrofin]

In D139644#3986488, @IBricchi wrote:

We will see if @phosek has time, but for context we imitated https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt for the cmake changes.

OK, but why did you need to make everything LLVM_OPTIONAL_SOURCES and not just the plugin-loading test?

[IBricchi]

The reason we need to do this is because otherwise we fail this check: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/LLVMProcessSources.cmake#L105

The check is making sure that there aren't any dead files in a folder, and it does this by making sure all files are used by all targets. However in our case we have two targets and each one uses a subset of the files so we have to use the LLVM_OPTIONAL_SOURCES to tell CMAKE that the files not being used by a given target aren't dead.

[mtrofin]

In D139644#3991463, @IBricchi wrote:

The reason we need to do this is because otherwise we fail this check: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/LLVMProcessSources.cmake#L105

The check is making sure that there aren't any dead files in a folder, and it does this by making sure all files are used by all targets. However in our case we have two targets and each one uses a subset of the files so we have to use the LLVM_OPTIONAL_SOURCES to tell CMAKE that the files not being used by a given target aren't dead.

Right, but my question is why does everything get made LLVM_OPTIONAL_SOURCES and not just the subset that's not used on the specific target?

[IBricchi]

Right I see, well there are two targets, the first is the unit tests which uses all the files except "InlineAdvisorPlugin.cpp", and then there's the target for the inlineAdvisorPlugin which only uses "InlineAdvisorPlugin.cpp", so for the first target we need "InlineAdvisorPlugin.cpp" in the optional source, and for the second target we need everything else.

[mtrofin]

In D139644#3992236, @IBricchi wrote:

Right I see, well there are two targets, the first is the unit tests which uses all the files except "InlineAdvisorPlugin.cpp", and then there's the target for the inlineAdvisorPlugin which only uses "InlineAdvisorPlugin.cpp", so for the first target we need "InlineAdvisorPlugin.cpp" in the optional source, and for the second target we need everything else.

Huh. Why would you not want to run the other tests when you run the plugin advisor? If it's because those tests don't work on that target, I'd recommend making that factoring first - i.e. just the cmakelists change(s) that exclude unwanted tests from that target.

[IBricchi]

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

[mtrofin]

In D139644#3999423, @IBricchi wrote:

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

OK, but that doesn't explain why it'd want to do the opposite for the test plugin target.

Also, my suggestion was to do that change - i.e. the change that makes conditional compilation of certain files (except for the to-be-introduced plugin unit test) in a separate patch, which would have to land first. This is so that the concerns are separated: one thing is about having a different build behavior on certain platforms. The other is adding the new unit test.

[ttheodor]

In D139644#3999600, @mtrofin wrote:

In D139644#3999423, @IBricchi wrote:

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

OK, but that doesn't explain why it'd want to do the opposite for the test plugin target.

Without setting LLVM_OPTIONAL_SOURCES before add_llvm_library(InlineAdvisorPlugin MODULE BUILDTREE_ONLY InlineAdvisorPlugin.cpp) we get the following error:

CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AliasAnalysisTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)


CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AliasSetTrackerTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)


CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AssumeBundleQueriesTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)

...
#Continues listing all other source files in the directory

The issue is that the add_llvm_library expects that all source files in the current directory are used by its target; it does this via llvm_check_source_file_list by scanning the current directory and checking the target's sources. llvm_check_source_file_list ignores files in LLVM_OPTIONAL_SOURCES and is meant for CMakeLists.txt that define multiple targets (other unit tests that define such plugins also do this). That's why we need to set all other files as optional sources before adding the plugin module.

In D139644#3999600, @mtrofin wrote:

Also, my suggestion was to do that change - i.e. the change that makes conditional compilation of certain files (except for the to-be-introduced plugin unit test) in a separate patch, which would have to land first. This is so that the concerns are separated: one thing is about having a different build behavior on certain platforms. The other is adding the new unit test.

The only conditionally built target is the plugin module, everything else is built normally as before. Note that LLVM_OPTIONAL_SOURCES is set after add_llvm_unittest_with_input_files(AnalysisTests ..., so the original unit test is not affected.

[mtrofin]

In D139644#4000660, @ttheodor wrote:

In D139644#3999600, @mtrofin wrote:

In D139644#3999423, @IBricchi wrote:

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

OK, but that doesn't explain why it'd want to do the opposite for the test plugin target.

Without setting LLVM_OPTIONAL_SOURCES before add_llvm_library(InlineAdvisorPlugin MODULE BUILDTREE_ONLY InlineAdvisorPlugin.cpp) we get the following error:

CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AliasAnalysisTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)


CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AliasSetTrackerTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)


CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AssumeBundleQueriesTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)

...
#Continues listing all other source files in the directory

The issue is that the add_llvm_library expects that all source files in the current directory are used by its target; it does this via llvm_check_source_file_list by scanning the current directory and checking the target's sources. llvm_check_source_file_list ignores files in LLVM_OPTIONAL_SOURCES and is meant for CMakeLists.txt that define multiple targets (other unit tests that define such plugins also do this). That's why we need to set all other files as optional sources before adding the plugin module.

My main question is why you needed to have a target that has only the plugin - I think the answer is that it's because you're building 2 targets now from the same cmake lists, and the second one, the InlineAdvisorPlugin, should just have the plugin, correct?

How about:

• define a variable, e.g. ANALYSIS_TEST_SOURCES, that lists all the sources at lines 26-66
• then: add_llvm_unittest_with_input_files(AnalysisTests ${ANALYSIS_TEST_SOURCES}) and then you don't need to re-list all the sources again (and, thus, avoid maintenance issues), and just concatenate to LLVM_OPTIONAL_SOURCES the whole ANALYSIS_TEST_SOURCE list and please add a comment as to why this happens (the reason above: so you can build the second, stand-alone plugin)

I'd also suggest this second bit happened *only* when you build the plugin, that would make things more clear - so under the line 119.

I think that would both keep the file maintainable - when tests need to be added, they just need to be added to the ANALYSIS_TEST_SOURCE - and clarify that the expansion of LLVM_OPTIONAL_SOURCES only happens if and only if you need to build the extra plugin standalone library.

wdyt?

In D139644#3999600, @mtrofin wrote:

Also, my suggestion was to do that change - i.e. the change that makes conditional compilation of certain files (except for the to-be-introduced plugin unit test) in a separate patch, which would have to land first. This is so that the concerns are separated: one thing is about having a different build behavior on certain platforms. The other is adding the new unit test.

The only conditionally built target is the plugin module, everything else is built normally as before. Note that LLVM_OPTIONAL_SOURCES is set after add_llvm_unittest_with_input_files(AnalysisTests ..., so the original unit test is not affected.

llvm/unittests/Analysis/CMakeLists.txt

10

Use include/llvm/Config/llvm-config.h.cmake instead define the preprocessor macro.

[IBricchi]

I think your suggestions make sense, I've updated the cmake file to address those points.

[mtrofin]

In D139644#4001765, @IBricchi wrote:

I think your suggestions make sense, I've updated the cmake file to address those points.

Thanks, looks good - just the thing about llvm-config.h.cmake, it's the canonical way to "convert" cmake flags to macro directives - so you wouldn't need to do :

if (LLVM_ENABLE_PLUGINS)
  add_definitions(-DLLVM_ENABLE_PLUGINS)
endif()

and, added bonus, others in the future that would want to #ifdef LLVM_ENABLE_PLUGINS would be able to do so.

llvm/unittests/Analysis/CMakeLists.txt

68

see the early comment about using llvm-configlh.cmake here

[IBricchi]

I must have missed that comment in my previous pass, I've now updated the llvm-config.h.cmake file

[IBricchi]

I don't have commit access to LLVM, would it be possible for you to commit it for me @mtrofin, or is there a different process I need to take?

[mtrofin]

In D139644#4002243, @IBricchi wrote:

I don't have commit access to LLVM, would it be possible for you to commit it for me @mtrofin, or is there a different process I need to take?

I'll do it.

[mtrofin]

In D139644#4002279, @mtrofin wrote:

In D139644#4002243, @IBricchi wrote:

I don't have commit access to LLVM, would it be possible for you to commit it for me @mtrofin, or is there a different process I need to take?

I'll do it.

@IBricchi , you need to rebase the patch out off origin/main (i.e. git branch -vv should say origin/main for your branch). Can you please do that and update it - then I can arc patch properly. Thanks!

OK, that worked, but looks like the author wasn't set in your commit. What should I set it to? (i.e. name and email)

[IBricchi]

Updated.
Author: ibricchi
Email: [e-mail]

[mtrofin]

In D139644#4002530, @IBricchi wrote:

Updated.
Author: ibricchi
Email: [e-mail]

perfect - btw, when you rebased, the llvm-config.h change got lost, can you please fix? thanks!

[IBricchi]

My bad, should be fixed now

[mtrofin]

In D139644#4002606, @IBricchi wrote:

My bad, should be fixed now

Thanks

Have you tried building from scratch - i.e. rm -rf your build dir, and start from cmake - because I'm getting this:

/other/llvm-project/llvm/include/llvm/IR/Attributes.h:90:14: fatal error: 'llvm/IR/Attributes.inc' file not found
    #include "llvm/IR/Attributes.inc"

[IBricchi]

I tried and I couldn't replicate what you described. Would you be able to say what cmake options you're using so I can try those exactly

[mtrofin]

In D139644#4002653, @IBricchi wrote:

I tried and I couldn't replicate what you described. Would you be able to say what cmake options you're using so I can try those exactly

sorted out - error on my side. sorry about the false alarm.

...or so I thought. My local fix was not specifying LLVM_BINUTILS_INCDIR, but it seems that was just hiding the problem. I attached to the revert commit message a few build bot links that should give sufficient context for a repro.

[IBricchi]

I managed to re-create the error and I think it was caused by the order in which things were being built. I added a dependancy for the plugin so that Attributes.inc is ready by the time the plugin comes around to being built.

[mtrofin]

llvm/unittests/Analysis/CMakeLists.txt

60

note: I got rid of the EXTRA_TESTS since having ANALYSIS_TEST_SOURCES removes the need for an extra list: e can just append to the latter.

105

Ack.

[akyrtzi]

Hi @IBricchi, the compiler error is still occurring (as of 1a9dec0dda0064cd0035d70b6534a65856b7a212).
To reproduce create a clean build directory (configure using ninja) and run ninja unittests/Analysis/CMakeFiles/InlineAdvisorPlugin.dir/InlineAdvisorPlugin.cpp.o.

[IBricchi]

I've created a patch which should solve the issue.

https://reviews.llvm.org/D140266

I took the change from:
https://github.com/llvm/llvm-project/blob/7d2c1150d31bb3692c81f6ecb0dbe61170e31212/llvm/unittests/Passes/CMakeLists.txt

[thakis]

The CMake setup here is pretty unusual. LLVM uses one target per cmake file almost everywhere, and many of the macros expect that. Maybe the plugin could be in a plugin/ subdir of the test and that might make things easier? clang/lib/Analysis/plugins/ has examples of that (which are loaded via lit).

(The two targets in unittests/Passes/CMakeLists.txt are _also_ unusual.)

Thanks, looks good - just the thing about llvm-config.h.cmake, it's the canonical way to "convert" cmake flags to macro directives - so you wouldn't need to do :

That's not fully true. For things that are internal to llvm, that place is config.h I think. llvm-config.h is for places outside of LLVM.

(The two targets in unittests/Passes/CMakeLists.txt are _also_ unusual.)(The two targets in unittests/Passes/CMakeLists.txt are _also_ unusual.)

(And it's kind of not a great place since it means toggling a single cmake option means rebuilding all files that read any cmake setting, but it's the current setup.)

[thakis]

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

By the way, was there an RFC somewhere that discussed why this is a good thing for the project? (I have no opinion on this myself, but it clearly comes with a cost, so it'd be good if people working in this domain overall agreed that this is a good direction.)

[thakis]

llvm/unittests/Analysis/CMakeLists.txt

10

Also, why did 7d2c1150d31bb remove the comment together with the define? The comment seems useful independent of the define.

[thakis]

llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp

65

This test is added unconditionally and doesn't check LLVM_ENABLE_PLUGINS as far as I can tell. How does this work with plugins disabled?

[thakis]

Also, I'm getting Could not load library '...InlineAdvisorPlugin.so': ...InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm27PluginInlineAdvisorAnalysis17HasBeenRegisteredE when running the test. That's probably because the plugin includes the file that declares that file-level static but then doesn't have a definition of it in the linkage unit (?)

[thakis]

llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp

65

The check for LLVM_ENABLE_PLUGINS is below on line 262. Sorry for the noise.

[mtrofin]

llvm/unittests/Analysis/CMakeLists.txt

10

As the define becomes available part of llvm-config.h (or config.h - separate issue), there's nothing left to comment on in the cmake, and there's a comment to the same effect in PluginsTest.cpp.

[thakis]

In D139644#4003708, @thakis wrote:

Also, I'm getting Could not load library '...InlineAdvisorPlugin.so': ...InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm27PluginInlineAdvisorAnalysis17HasBeenRegisteredE when running the test. That's probably because the plugin includes the file that declares that file-level static but then doesn't have a definition of it in the linkage unit (?)

This was due to me not linking AnalysisTests with -rdynamic.

Things work now over here.

The "this is unusual" comment remains :)

[mtrofin]

In D139644#4004024, @thakis wrote:

In D139644#4003708, @thakis wrote:

Also, I'm getting Could not load library '...InlineAdvisorPlugin.so': ...InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm27PluginInlineAdvisorAnalysis17HasBeenRegisteredE when running the test. That's probably because the plugin includes the file that declares that file-level static but then doesn't have a definition of it in the linkage unit (?)

This was due to me not linking AnalysisTests with -rdynamic.

Things work now over here.

The "this is unusual" comment remains :)

Can we help @IBricchi with a recommendation as to how to author a test - for example, perhaps having a subdirectory for the test plugin?

[IBricchi]

The reason we went with this general structure for the cmake file is that the other unit tests that implement similar functionality implement it like this:

https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt

[tmatheson]

I am still seeing this linking error when building with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS: /unittests/Analysis/InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyEStack.

[thakis]

In D139644#4003692, @thakis wrote:

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

By the way, was there an RFC somewhere that discussed why this is a good thing for the project? (I have no opinion on this myself, but it clearly comes with a cost, so it'd be good if people working in this domain overall agreed that this is a good direction.)

BTW, was this ever answered?

[IBricchi]

In D139644#4015049, @tmatheson wrote:

I am still seeing this linking error when building with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS: /unittests/Analysis/InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyEStack.

I can't seem to get this to happen, and the build-bots on main don't seem to encounter it either.

Would you be able to tell me how to replicate this?

[IBricchi]

In D139644#4015314, @thakis wrote:

In D139644#4003692, @thakis wrote:

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

By the way, was there an RFC somewhere that discussed why this is a good thing for the project? (I have no opinion on this myself, but it clearly comes with a cost, so it'd be good if people working in this domain overall agreed that this is a good direction.)

BTW, was this ever answered?

Usefulness: In our group (and this applies to others as well) we have been working on inlining for a while. However our work is experimental/research, and it makes little sense to try to upstream it directly on LLVM. his creates a barrier for sharing it with the community so that it can be used and evaluated. LLVM already had the correct abstraction to solve our problem, the InliningAdvisor; all that was missing was what this patch to enable the ability to externally provide one. This is greatly reduces the barrier for sharing inlining (heuristic) related work, not only for our group, but anyone who's working on that.

Cost: the only runtime cost is a boolean check, whether a PluginAdvisor has been registered, which should not even be measurable.

llvm/include/llvm/Analysis/InlineAdvisor.h

 class CallBase;
 class Function;
 class Module;
 class OptimizationRemark;
 class ImportedFunctionsInliningStatistics;
 class OptimizationRemarkEmitter;
 struct ReplayInlinerSettings;
 
-/// There are 3 scenarios we can use the InlineAdvisor:
+/// There are 4 scenarios we can use the InlineAdvisor:
 /// - Default - use manual heuristics.
 ///
 /// - Release mode, the expected mode for production, day to day deployments.
 /// In this mode, when building the compiler, we also compile a pre-trained ML
 /// model to native code, and link it as a static library. This mode has low
 /// overhead and no additional dependencies for the compiler runtime.
 ///
 /// - Development mode, for training new models.
 /// In this mode, we trade off runtime performance for flexibility. This mode
 /// requires the full C Tensorflow API library, and evaluates models
 /// dynamically. This mode also permits generating training logs, for offline
 /// training.
+///
+/// - Dynamically load an advisor via a plugin (PluginInlineAdvisorAnalysis)
 enum class InliningAdvisorMode : int { Default, Release, Development };
 
 // Each entry represents an inline driver.
 enum class InlinePass : int {
   AlwaysInliner,
   CGSCCInliner,
   EarlyInliner,
   ModuleInliner,
DefaultInlineAdvisor(Module &M, FunctionAnalysisManager &FAM,
       : InlineAdvisor(M, FAM, IC), Params(Params) {}
 
 private:
   std::unique_ptr<InlineAdvice> getAdviceImpl(CallBase &CB) override;
 
   InlineParams Params;
 };
 
+/// Used for dynamically registering InlineAdvisors as plugins
+///
+/// An advisor plugin adds a new advisor at runtime by registering an instance
+/// of PluginInlineAdvisorAnalysis in the current ModuleAnalysisManager.
+/// For example, the following code dynamically registers a
+/// DefaultInlineAdvisor:
+///
+/// namespace {
+///
+/// InlineAdvisor *defaultAdvisorFactory(Module &M, FunctionAnalysisManager
+/// &FAM,
+///                                      InlineParams Params, InlineContext IC)
+///                                      {
+///   return new DefaultInlineAdvisor(M, FAM, Params, IC);
+/// }
+///
+/// struct DefaultDynamicAdvisor : PassInfoMixin<DefaultDynamicAdvisor> {
+///   PreservedAnalyses run(Module &, ModuleAnalysisManager &MAM) {
+///     PluginInlineAdvisorAnalysis PA(defaultAdvisorFactory);
+///     MAM.registerPass([&] { return PA; });
+///     return PreservedAnalyses::all();
+///   }
+/// };
+///
+/// } // namespace
+///
+/// extern "C" LLVM_ATTRIBUTE_WEAK ::llvm::PassPluginLibraryInfo
+/// llvmGetPassPluginInfo() {
+///   return {LLVM_PLUGIN_API_VERSION, "DynamicDefaultAdvisor",
+///   LLVM_VERSION_STRING,
+///           [](PassBuilder &PB) {
+///             PB.registerPipelineStartEPCallback(
+///                 [](ModulePassManager &MPM, OptimizationLevel Level) {
+///                   MPM.addPass(DefaultDynamicAdvisor());
+///                 });
+///           }};
+/// }
+///
+/// A plugin must implement an AdvisorFactory and register it with a
+/// PluginInlineAdvisorAnlysis to the provided ModuleanAlysisManager.
+///
+/// If such a plugin has been registered
+/// InlineAdvisorAnalysis::Result::tryCreate will return the dynamically loaded
+/// advisor.
+///
+class PluginInlineAdvisorAnalysis
+    : public AnalysisInfoMixin<PluginInlineAdvisorAnalysis> {
+public:
+  static AnalysisKey Key;
+  static bool HasBeenRegistered;
+
+  typedef InlineAdvisor *(*AdvisorFactory)(Module &M,
+                                           FunctionAnalysisManager &FAM,
+                                           InlineParams Params,
+                                           InlineContext IC);
+
+  PluginInlineAdvisorAnalysis(AdvisorFactory Factory) : Factory(Factory) {
+    HasBeenRegistered = true;
+    assert(Factory != nullptr &&
+           "The plugin advisor factory should not be a null pointer.");
+  }
+
+  struct Result {
+    AdvisorFactory Factory;
+  };
+
+  Result run(Module &M, ModuleAnalysisManager &MAM) { return {Factory}; }
+  Result getResult() { return {Factory}; }
+
+private:
+  AdvisorFactory Factory;
+};
+
 /// The InlineAdvisorAnalysis is a module pass because the InlineAdvisor
 /// needs to capture state right before inlining commences over a module.
 class InlineAdvisorAnalysis : public AnalysisInfoMixin<InlineAdvisorAnalysis> {
 public:
   static AnalysisKey Key;
   InlineAdvisorAnalysis() = default;
   struct Result {
     Result(Module &M, ModuleAnalysisManager &MAM) : M(M), MAM(MAM) {}

llvm/lib/Analysis/InlineAdvisor.cpp

 
 void InlineAdvice::recordInliningWithCalleeDeleted() {
   markRecorded();
   recordInlineStatsIfNeeded();
   recordInliningWithCalleeDeletedImpl();
 }
 
 AnalysisKey InlineAdvisorAnalysis::Key;
+AnalysisKey PluginInlineAdvisorAnalysis::Key;
+bool PluginInlineAdvisorAnalysis::HasBeenRegistered = false;
 
 bool InlineAdvisorAnalysis::Result::tryCreate(
     InlineParams Params, InliningAdvisorMode Mode,
     const ReplayInlinerSettings &ReplaySettings, InlineContext IC) {
   auto &FAM = MAM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
+  if (PluginInlineAdvisorAnalysis::HasBeenRegistered) {
+    auto &DA = MAM.getResult<PluginInlineAdvisorAnalysis>(M);
+    Advisor.reset(DA.Factory(M, FAM, Params, IC));
+    return !!Advisor;
+  }
   switch (Mode) {
   case InliningAdvisorMode::Default:
     LLVM_DEBUG(dbgs() << "Using default inliner heuristic.\n");
     Advisor.reset(new DefaultInlineAdvisor(M, FAM, Params, IC));
     // Restrict replay to default advisor, ML advisors are stateful so
     // replay will need augmentations to interleave with them correctly.
     if (!ReplaySettings.ReplayFile.empty()) {
       Advisor = llvm::getReplayInlineAdvisor(M, FAM, M.getContext(),

llvm/unittests/Analysis/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Analysis
   AsmParser
   Core
+  Passes
   Support
   TransformUtils
   )
 
-set(MLGO_TESTS TFUtilsTest.cpp TrainingLoggerTest.cpp)
-if (LLVM_HAVE_TFLITE)
-  LIST(APPEND EXTRA_TESTS ${MLGO_TESTS})
-else()
-  LIST(APPEND LLVM_OPTIONAL_SOURCES ${MLGO_TESTS})
-endif()
-
-add_llvm_unittest_with_input_files(AnalysisTests
+set(ANALYSIS_TEST_SOURCES
   AliasAnalysisTest.cpp
   AliasSetTrackerTest.cpp
   AssumeBundleQueriesTest.cpp
   BasicAliasAnalysisTest.cpp
   BlockFrequencyInfoTest.cpp
   BranchProbabilityInfoTest.cpp
   CallGraphTest.cpp
   CaptureTrackingTest.cpp
set(ANALYSIS_TEST_SOURCES
   LoadsTest.cpp
   LoopInfoTest.cpp
   LoopNestTest.cpp
   MemoryBuiltinsTest.cpp
   MemoryProfileInfoTest.cpp
   MemorySSATest.cpp
   MLModelRunnerTest.cpp
   PhiValuesTest.cpp
+  PluginInlineAdvisorAnalysisTest.cpp
   ProfileSummaryInfoTest.cpp
   ScalarEvolutionTest.cpp
   VectorFunctionABITest.cpp
   SparsePropagation.cpp
   TargetLibraryInfoTest.cpp
   TensorSpecTest.cpp
   TBAATest.cpp
   UnrollAnalyzerTest.cpp
   ValueLatticeTest.cpp
   ValueTrackingTest.cpp
   VectorUtilsTest.cpp
+  )
+
+# The unit tests target does not use InlineAdvisorPlugin.cpp
+# so if not added to LLVM_OPTIONAL_SOURCES, FileCheck will
+# complain about unused file.
+set(LLVM_OPTIONAL_SOURCES InlineAdvisorPlugin.cpp)
+
+set(MLGO_TESTS TFUtilsTest.cpp TrainingLoggerTest.cpp)
+if (LLVM_HAVE_TFLITE)
+  LIST(APPEND EXTRA_TESTS ${MLGO_TESTS})
+else()
+  LIST(APPEND LLVM_OPTIONAL_SOURCES ${MLGO_TESTS})
+endif()
+
+# If plugins are disabled, the PluginInlineAdvisorTest will disable itself at runtime. 
+# Otherwise, reconfiguring with plugins disabled will leave behind a stale executable.
+if (LLVM_ENABLE_PLUGINS)
+  add_definitions(-DLLVM_ENABLE_PLUGINS)
+endif()
+
+add_llvm_unittest_with_input_files(AnalysisTests
+  ${ANALYSIS_TEST_SOURCES}
   ${EXTRA_TESTS}
   )
 
- target_link_libraries(AnalysisTests PRIVATE LLVMTestingSupport)
+
+# The advisor plugin expects to not link against the Analysis, Support and Core 
+# libraries, but expects them to exist in the process loading the plugin. This 
+# doesn't work with DLLs on Windows (where a shared library can't have undefined
+# references), so just skip this testcase on Windows.
+if (NOT WIN32)
+  # On AIX, enable run-time linking to allow symbols from the plugins shared
+  # objects to be properly bound.
+  if(CMAKE_SYSTEM_NAME STREQUAL "AIX")
+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-brtl")
+  endif()
+
+  # This plugin is built as a stand-alone plugin, so since we don't use the
+  # ANALYSIS_TEST_SOURCES files, we have to add them to LLVM_OPTIONAL_SOURCES
+  # so that FileCheck doesn't complain about unsued files.
+  LIST(APPEND LLVM_OPTIONAL_SOURCES ${ANALYSIS_TEST_SOURCES})
+
+  unset(LLVM_LINK_COMPONENTS)
+  add_llvm_library(InlineAdvisorPlugin MODULE BUILDTREE_ONLY InlineAdvisorPlugin.cpp)
+  # Put PLUGIN next to the unit test executable.
+  set_output_directory(InlineAdvisorPlugin
+      BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+      LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+      )
+  set_target_properties(InlineAdvisorPlugin PROPERTIES FOLDER "Tests")
+
+  export_executable_symbols_for_plugins(AnalysisTests)
+  add_dependencies(AnalysisTests InlineAdvisorPlugin)
+  set_property(TARGET InlineAdvisorPlugin PROPERTY FOLDER "Tests/UnitTests/AnalysisTests")
+endif()

llvm/unittests/Analysis/InlineAdvisorPlugin.cpp

+#include "llvm/IR/Function.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Pass.h"
+#include "llvm/Passes/PassBuilder.h"
+#include "llvm/Passes/PassPlugin.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include "llvm/Analysis/InlineAdvisor.h"
+
+using namespace llvm;
+
+namespace {
+
+InlineAdvisor *DefaultAdvisorFactory(Module &M, FunctionAnalysisManager &FAM,
+                                     InlineParams Params, InlineContext IC) {
+  return new DefaultInlineAdvisor(M, FAM, Params, IC);
+}
+
+struct DefaultDynamicAdvisor : PassInfoMixin<DefaultDynamicAdvisor> {
+  PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM) {
+    PluginInlineAdvisorAnalysis DA(DefaultAdvisorFactory);
+    MAM.registerPass([&] { return DA; });
+    return PreservedAnalyses::all();
+  }
+};
+
+} // namespace
+
+/* New PM Registration */
+llvm::PassPluginLibraryInfo getDefaultDynamicAdvisorPluginInfo() {
+  return {LLVM_PLUGIN_API_VERSION, "DynamicDefaultAdvisor", LLVM_VERSION_STRING,
+          [](PassBuilder &PB) {
+            PB.registerPipelineStartEPCallback(
+                [](ModulePassManager &MPM, OptimizationLevel Level) {
+                  MPM.addPass(DefaultDynamicAdvisor());
+                });
+            PB.registerPipelineParsingCallback(
+                [](StringRef Name, ModulePassManager &PM,
+                   ArrayRef<PassBuilder::PipelineElement> InnerPipeline) {
+                  if (Name == "dynamic-inline-advisor") {
+                    PM.addPass(DefaultDynamicAdvisor());
+                    return true;
+                  }
+                  return false;
+                });
+          }};
+}
+
+extern "C" LLVM_ATTRIBUTE_WEAK ::llvm::PassPluginLibraryInfo
+llvmGetPassPluginInfo() {
+  return getDefaultDynamicAdvisorPluginInfo();
+}

llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp

+#include "llvm/Analysis/CallGraph.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/Config/config.h"
+#include "llvm/Passes/PassBuilder.h"
+#include "llvm/Passes/PassPlugin.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+namespace llvm {
+
+void anchor() {}
+
+static std::string libPath(const std::string Name = "InlineAdvisorPlugin") {
+  const auto &Argvs = testing::internal::GetArgvs();
+  const char *Argv0 =
+      Argvs.size() > 0 ? Argvs[0].c_str() : "PluginInlineAdvisorAnalysisTest";
+  void *Ptr = (void *)(intptr_t)anchor;
+  std::string Path = sys::fs::getMainExecutable(Argv0, Ptr);
+  llvm::SmallString<256> Buf{sys::path::parent_path(Path)};
+  sys::path::append(Buf, (Name + LLVM_PLUGIN_EXT).c_str());
+  return std::string(Buf.str());
+}
+
+// Example of a custom InlineAdvisor that only inlines calls to functions called
+// "foo".
+class FooOnlyInlineAdvisor : public InlineAdvisor {
+public:
+  FooOnlyInlineAdvisor(Module &M, FunctionAnalysisManager &FAM,
+                       InlineParams Params, InlineContext IC)
+      : InlineAdvisor(M, FAM, IC) {}
+
+  std::unique_ptr<InlineAdvice> getAdviceImpl(CallBase &CB) override {
+    if (CB.getCalledFunction()->getName() == "foo")
+      return std::make_unique<InlineAdvice>(this, CB, getCallerORE(CB), true);
+    return std::make_unique<InlineAdvice>(this, CB, getCallerORE(CB), false);
+  }
+};
+
+static InlineAdvisor *fooOnlyFactory(Module &M, FunctionAnalysisManager &FAM,
+                                     InlineParams Params, InlineContext IC) {
+  return new FooOnlyInlineAdvisor(M, FAM, Params, IC);
+}
+
+struct CompilerInstance {
+  LLVMContext Ctx;
+  ModulePassManager MPM;
+  InlineParams IP;
+
+  PassBuilder PB;
+  LoopAnalysisManager LAM;
+  FunctionAnalysisManager FAM;
+  CGSCCAnalysisManager CGAM;
+  ModuleAnalysisManager MAM;
+
+  SMDiagnostic Error;
+
+  // connect the plugin to our compiler instance
+  void setupPlugin() {
+    auto PluginPath = libPath();
+    ASSERT_NE("", PluginPath);
+    Expected<PassPlugin> Plugin = PassPlugin::Load(PluginPath);
+    ASSERT_TRUE(!!Plugin) << "Plugin path: " << PluginPath;
+    Plugin->registerPassBuilderCallbacks(PB);
+    ASSERT_THAT_ERROR(PB.parsePassPipeline(MPM, "dynamic-inline-advisor"),
+                      Succeeded());
+  }
+
+  // connect the FooOnlyInlineAdvisor to our compiler instance
+  void setupFooOnly() {
+    MAM.registerPass(
+        [&] { return PluginInlineAdvisorAnalysis(fooOnlyFactory); });
+  }
+
+  CompilerInstance() {
+    IP = getInlineParams(3, 0);
+    PB.registerModuleAnalyses(MAM);
+    PB.registerCGSCCAnalyses(CGAM);
+    PB.registerFunctionAnalyses(FAM);
+    PB.registerLoopAnalyses(LAM);
+    PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
+    MPM.addPass(ModuleInlinerPass(IP, InliningAdvisorMode::Default,
+                                  ThinOrFullLTOPhase::None));
+  }
+
+  std::string output;
+  std::unique_ptr<Module> outputM;
+
+  // run with the default inliner
+  auto run_default(StringRef IR) {
+    PluginInlineAdvisorAnalysis::HasBeenRegistered = false;
+    outputM = parseAssemblyString(IR, Error, Ctx);
+    MPM.run(*outputM, MAM);
+    ASSERT_TRUE(outputM);
+    output.clear();
+    raw_string_ostream o_stream{output};
+    outputM->print(o_stream, nullptr);
+    ASSERT_TRUE(true);
+  }
+
+  // run with the dnamic inliner
+  auto run_dynamic(StringRef IR) {
+    // note typically the constructor for the DynamicInlineAdvisorAnalysis
+    // will automatically set this to true, we controll it here only to
+    // altenate between the default and dynamic inliner in our test
+    PluginInlineAdvisorAnalysis::HasBeenRegistered = true;
+    outputM = parseAssemblyString(IR, Error, Ctx);
+    MPM.run(*outputM, MAM);
+    ASSERT_TRUE(outputM);
+    output.clear();
+    raw_string_ostream o_stream{output};
+    outputM->print(o_stream, nullptr);
+    ASSERT_TRUE(true);
+  }
+};
+
+StringRef TestIRS[] = {
+    // Simple 3 function inline case
+    R"(
+define void @f1() {
+  call void @foo()
+  ret void
+}
+define void @foo() {
+  call void @f3()
+  ret void
+}
+define void @f3() {
+  ret void
+}
+  )",
+    // Test that has 5 functions of which 2 are recursive
+    R"(
+define void @f1() {
+  call void @foo()
+  ret void
+}
+define void @f2() {
+  call void @foo()
+  ret void
+}
+define void @foo() {
+  call void @f4()
+  call void @f5()
+  ret void
+}
+define void @f4() {
+  ret void
+}
+define void @f5() {
+  call void @foo()
+  ret void
+}
+  )",
+    // test with 2 mutually recursive functions and 1 function with a loop
+    R"(
+define void @f1() {
+  call void @f2()
+  ret void
+}
+define void @f2() {
+  call void @f3()
+  ret void
+}
+define void @f3() {
+  call void @f1()
+  ret void
+}
+define void @f4() {
+  br label %loop
+loop:
+  call void @f5()
+  br label %loop
+}
+define void @f5() {
+  ret void
+}
+  )",
+    // test that has a function that computes fibonacci in a loop, one in a
+    // recurisve manner, and one that calls both and compares them
+    R"(
+define i32 @fib_loop(i32 %n){
+    %curr = alloca i32
+    %last = alloca i32
+    %i = alloca i32
+    store i32 1, i32* %curr
+    store i32 1, i32* %last
+    store i32 2, i32* %i
+    br label %loop_cond
+  loop_cond:
+    %i_val = load i32, i32* %i
+    %cmp = icmp slt i32 %i_val, %n
+    br i1 %cmp, label %loop_body, label %loop_end
+  loop_body:
+    %curr_val = load i32, i32* %curr
+    %last_val = load i32, i32* %last
+    %add = add i32 %curr_val, %last_val
+    store i32 %add, i32* %last
+    store i32 %curr_val, i32* %curr
+    %i_val2 = load i32, i32* %i
+    %add2 = add i32 %i_val2, 1
+    store i32 %add2, i32* %i
+    br label %loop_cond
+  loop_end:
+    %curr_val3 = load i32, i32* %curr
+    ret i32 %curr_val3
+}
+
+define i32 @fib_rec(i32 %n){
+    %cmp = icmp eq i32 %n, 0
+    %cmp2 = icmp eq i32 %n, 1
+    %or = or i1 %cmp, %cmp2
+    br i1 %or, label %if_true, label %if_false
+  if_true:
+    ret i32 1
+  if_false:
+    %sub = sub i32 %n, 1
+    %call = call i32 @fib_rec(i32 %sub)
+    %sub2 = sub i32 %n, 2
+    %call2 = call i32 @fib_rec(i32 %sub2)
+    %add = add i32 %call, %call2
+    ret i32 %add
+}
+
+define i32 @fib_check(){
+    %correct = alloca i32
+    %i = alloca i32
+    store i32 1, i32* %correct
+    store i32 0, i32* %i
+    br label %loop_cond
+  loop_cond:
+    %i_val = load i32, i32* %i
+    %cmp = icmp slt i32 %i_val, 10
+    br i1 %cmp, label %loop_body, label %loop_end
+  loop_body:
+    %i_val2 = load i32, i32* %i
+    %call = call i32 @fib_loop(i32 %i_val2)
+    %i_val3 = load i32, i32* %i
+    %call2 = call i32 @fib_rec(i32 %i_val3)
+    %cmp2 = icmp ne i32 %call, %call2
+    br i1 %cmp2, label %if_true, label %if_false
+  if_true:
+    store i32 0, i32* %correct
+    br label %if_end
+  if_false:
+    br label %if_end
+  if_end:
+    %i_val4 = load i32, i32* %i
+    %add = add i32 %i_val4, 1
+    store i32 %add, i32* %i
+    br label %loop_cond
+  loop_end:
+    %correct_val = load i32, i32* %correct
+    ret i32 %correct_val
+}
+  )"};
+
+// check that loading a plugin works
+// the plugin being loaded acts identically to the default inliner
+TEST(PluginInlineAdvisorTest, PluginLoad) {
+#if !defined(LLVM_ENABLE_PLUGINS)
+  // Disable the test if plugins are disabled.
+  return;
+#endif
+  CompilerInstance CI{};
+  CI.setupPlugin();
+
+  for (StringRef IR : TestIRS) {
+    CI.run_default(IR);
+    std::string default_output = CI.output;
+    CI.run_dynamic(IR);
+    std::string dynamic_output = CI.output;
+    ASSERT_EQ(default_output, dynamic_output);
+  }
+}
+
+// check that the behaviour of a custom inliner is correct
+// the custom inliner inlines all functions that are not named "foo"
+// this testdoes not require plugins to be enabled
+TEST(PluginInlineAdvisorTest, CustomAdvisor) {
+  CompilerInstance CI{};
+  CI.setupFooOnly();
+
+  for (StringRef IR : TestIRS) {
+    CI.run_dynamic(IR);
+    CallGraph CGraph = CallGraph(*CI.outputM);
+    for (auto &node : CGraph) {
+      for (auto &edge : *node.second) {
+        if (!edge.first)
+          continue;
+        ASSERT_NE(edge.second->getFunction()->getName(), "foo");
+      }
+    }
+  }
+}
+
+} // namespace llvm