[InlineOrder] Plugin Inline Order

[IBricchi]

Summary

Adds the ability to load a plugin to control the inline order.
This allows developing and distributing inlining heuristics
outside of tree. And together with the inline advisor plugins
allows for fine grained control of the inliner.

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

[IBricchi]

This patch builds off https://reviews.llvm.org/D139644 to add plugin support for inline order. This should allow for full fine grained control of the inliner using plugins.

[IBricchi]

rebase

[IBricchi]

format

[IBricchi]

Implement fix for AIX cmake issue from https://reviews.llvm.org/D140559

[IBricchi]

ping

[kazu]

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

llvm/include/llvm/Analysis/InlineOrder.h

40–43

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?

45–80

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.

83

ModuleAnalysisManager?

llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp

61–62

I am not familiar with this. Would you please explain? The new pass would load the plugin or something?

llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp

90–91

Could you capitalize the first letter of the sentence and put a period at the end. Easier to parse that way.

llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp

7

How are we using this?

19

Could we drop static inside the anonymous namespace?

64–65

Could you capitalize the first letter of the sentence and put a period at the end. Easier to parse that way.

[IBricchi]

[IBricchi]

[IBricchi]

Thanks for the review! I updated the patch to reflect the comments.

llvm/include/llvm/Analysis/InlineOrder.h

40–43

An instance of PluginInlineOrderAnalysis only exists if it has been registered, in a normal compilation it won't exist. If it's registered then indeed it's Factory will be used to create a custom order
We use an analysis here because we need the PassBuilder's ability to dynamically load passes as plugins (else we'd need to implement our own custom mechanism).

llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp

61–62

In the initial commit there was an unnecessary level of indirection, it has been fixed now. The plugin should just register a PluginInlineOrderAnalysis with the ModuleAnalysisManager.

[kazu]

Thank you for explaining how things work.

llvm/include/llvm/Analysis/InlineOrder.h

52

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.

llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp

20–24

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);
}

28

May I suggest the following?

// Ignore callees named "foo".

45

May I suggest getNoFooInlineOrder or noFooInlineOrderFactory? At least, I would drop default here because it's not the default inline order.

llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp

239–240

nit: Could we say named instead of called to avoid confusion when discussing inlining?

[IBricchi]

Thanks for the suggestions

[kazu]

Thank you for updating the patch. LGTM.

[IBricchi]

Ignore tests on windows due to limited pluign suport

[IBricchi]

I don't have commit access, could you commit it for me? ibricchi <[e-mail]>

[Jake-Egan]

Hi, we're seeing a CMake error on AIX after this change. Can you take a look please?

https://lab.llvm.org/buildbot/#/builders/214/builds/6366/steps/4/logs/stdio

[IBricchi]

I think the issue was that:

export_executable_symbols_for_plugins(AnalysisTests)

Was being called twice, once in each of the plugin cmake files. I've moved it into the main Analysys CMakeFile and I think this should fix the issue we were seeing, but I don't have an AIX machine to test it on. Would you mind checking @Jake-Egan?

[Jake-Egan]

In D140637#4196543, @IBricchi wrote:

I think the issue was that:

export_executable_symbols_for_plugins(AnalysisTests)

Was being called twice, once in each of the plugin cmake files. I've moved it into the main Analysys CMakeFile and I think this should fix the issue we were seeing, but I don't have an AIX machine to test it on. Would you mind checking @Jake-Egan?

It resolves the CMake error - thanks for the fix!

[IBricchi]

Glad to hear that! Would you mind pushing the fix? I don't have commit access, thanks