[PATCH] D140637: [InlineOrder] Plugin Inline Order

[Kazu Hirata via Phabricator via llvm-commits]

kazu added a comment.

Thank you for explaining how things work.



================
Comment at: llvm/include/llvm/Analysis/InlineOrder.h:52
+  static AnalysisKey Key;
+  static bool HasBeenRegistered;
+
----------------
Could we make this variable private and provide accessors like so?

```
static bool isRegistered() { return HasBeenRegistered; }
static void unregister() { HasBeenRegistered = false; }
```

I am suggesting this because I don't want anyone to set `HasBeenRegistered` to true except in the constructor here.


================
Comment at: llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp:19-23
+    // we temporarily set the PluginInlineOrderAnalysis::HasBeenRegistered
+    // as false to disable the plugin loading itself and renable it later
+    llvm::PluginInlineOrderAnalysis::HasBeenRegistered = false;
+    DefaultInlineOrder = getInlineOrder(FAM, Params, MAM, M);
+    llvm::PluginInlineOrderAnalysis::HasBeenRegistered = true;
----------------
Could we say something like this?

```
DefaultInlineOrder = getDefaultInlineOrder(FAM, Params, MAM, M);
```

To do this, we would need to rename `getInlineOrder` to `getDefaultInlineOrder`, turning `getInlineOrder` into a mere selector:

```
std::unique_ptr<InlineOrder<std::pair<CallBase *, int>>>
llvm::getDefaultInlineOrder(FunctionAnalysisManager &FAM, const InlineParams &Params,
                           ModuleAnalysisManager &MAM, Module &M) {
  // The original function body goes here.
}

std::unique_ptr<InlineOrder<std::pair<CallBase *, int>>>
llvm::getInlineOrder(FunctionAnalysisManager &FAM, const InlineParams &Params,
                     ModuleAnalysisManager &MAM, Module &M) {
  if (llvm::PluginInlineOrderAnalysis::HasBeenRegistered) {
    LLVM_DEBUG(dbgs() << "    Current used priority: plugin ---- \n");
    return MAM.getResult<PluginInlineOrderAnalysis>(M).Factory(FAM, Params, MAM,
                                                               M);
  }

  return getDefaultInlineOrder(FAM, Params, MAM, M);
}
```




================
Comment at: llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp:27
+  void push(const std::pair<CallBase *, int> &Elt) override {
+    // we ignore call that include foo in the name
+    if (Elt.first->getCalledFunction()->getName() == "foo") {
----------------
May I suggest the following?

```
// Ignore callees named "foo". 
```



================
Comment at: llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp:44
+std::unique_ptr<InlineOrder<std::pair<CallBase *, int>>>
+defaultInlineOrderFactory(FunctionAnalysisManager &FAM,
+                          const InlineParams &Params,
----------------
May I suggest `getNoFooInlineOrder` or `noFooInlineOrderFactory`?  At least, I would drop `default` here because it's not the default inline order.



================
Comment at: llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp:228-229
+// Check that the behaviour of a custom inline order is correct.
+// The custom order drops any functions called "foo" so all tests
+// should contain at least one function called foo.
+TEST(PluginInlineOrderTest, NoInlineFoo) {
----------------
nit: Could we say `named` instead of `called` to avoid confusion when discussing inlining?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140637