[PATCH] D140637: [InlineOrder] Plugin Inline Order

[Kazu Hirata via Phabricator via llvm-commits]

kazu added a comment.

I think I have a high-level understanding, but I haven't understood the implementation yet.



================
Comment at: llvm/include/llvm/Analysis/InlineOrder.h:40-43
+/// An inline order plugin adds a new inline order at runtime by registering
+/// an instance of PluginInlineOrderAnalysis in the current
+/// ModuleAnalysisManager. For example, the following code dynamically registers
+/// the default inline order:
----------------
Sorry, I am slow to understand this.  Is the following correct understanding?

`PluginInlineOrderAnalysis` is always there, regardless of whether you have a plugin.  If you actually have a plugin, we can use `Factory` in `PluginInlineOrderAnalysis` to construct a custom inline order.

Now, why are we using an analysis here?  Is that because it's a good hook to use?


================
Comment at: llvm/include/llvm/Analysis/InlineOrder.h:45-80
+/// namespace {
+///
+/// std::unique_ptr<InlineOrder<std::pair<CallBase *, int>>>
+/// DefaultInlineOrderFactory(FunctionAnalysisManager &FAM,
+///                           const InlineParams &Params,
+///                           ModuleAnalysisManager &MAM, Module &M) {
+///   llvm::PluginInlineOrderAnalysis::HasBeenRegistered = false;
----------------
Could we document the plugin as inline comments in `llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp` or something?  I am afraid that the big code block comments might diverge from what the actual code should look like.  If you are OK with this idea, you might want to put a pointer to the example file here.


================
Comment at: llvm/include/llvm/Analysis/InlineOrder.h:83
+/// A plugin must implement an InlineOrderFactory and register it with a
+/// PluginInlineOrderAnalysis to the provided ModuleanAlysisManager.
+///
----------------
`ModuleAnalysisManager`?


================
Comment at: llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp:60-61
+
+/* New PM Registration */
+llvm::PassPluginLibraryInfo getDefaultDynamicInlineOrderPluginInfo() {
+  return {LLVM_PLUGIN_API_VERSION, "DynamicDefaultInlineOrder",
----------------
I am not familiar with this.  Would you please explain?  The new pass would load the plugin or something?


================
Comment at: llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp:90-91
+  ~CompilerInstance() {
+    // reset the static variable that tracks if the plugin has been registered
+    // this is needed to allow the test to run multiple times
+    PluginInlineAdvisorAnalysis::HasBeenRegistered = false;
----------------
Could you capitalize the first letter of the sentence and put a period at the end.  Easier to parse that way.


================
Comment at: llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp:7
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/RandomNumberGenerator.h"
 #include "llvm/Support/raw_ostream.h"
----------------
How are we using this?


================
Comment at: llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp:18
+
+static void anchor() {}
 
----------------
Could we drop `static` inside the anonymous namespace?


================
Comment at: llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp:67-68
+  ~CompilerInstance() {
+    // reset the static variable that tracks if the plugin has been registered
+    // this is needed to allow the test to run multiple times
+    PluginInlineOrderAnalysis::HasBeenRegistered = false;
----------------
Could you capitalize the first letter of the sentence and put a period at the end.  Easier to parse that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140637