Page MenuHomePhabricator

[InlineOrder] Plugin Inline Order
ClosedPublic

Authored by IBricchi on Dec 23 2022, 2:12 PM.

Details

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.

Diff Detail

Event Timeline

IBricchi created this revision.Dec 23 2022, 2:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2022, 2:12 PM
IBricchi abandoned this revision.Dec 23 2022, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2022, 2:15 PM
IBricchi reclaimed this revision.Dec 23 2022, 2:16 PM
IBricchi updated this revision to Diff 485157.Dec 23 2022, 2:21 PM
IBricchi updated this revision to Diff 485158.
IBricchi published this revision for review.Dec 23 2022, 2:23 PM
IBricchi added reviewers: mtrofin, kazu.
IBricchi added a subscriber: ttheodor.

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 updated this revision to Diff 489738.Jan 17 2023, 1:57 AM
IBricchi updated this revision to Diff 498333.EditedFeb 17 2023, 4:31 AM

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

kazu added a comment.Wed, Mar 8, 9:56 AM

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 marked 5 inline comments as done.Fri, Mar 10, 10:57 AM

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 added a comment.Fri, Mar 10, 2:34 PM

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 updated this revision to Diff 504630.Mon, Mar 13, 6:45 AM
IBricchi marked 5 inline comments as done.

Thanks for the suggestions

kazu accepted this revision.Mon, Mar 13, 8:38 PM

Thank you for updating the patch. LGTM.

This revision is now accepted and ready to land.Mon, Mar 13, 8:38 PM
IBricchi updated this revision to Diff 504988.Tue, Mar 14, 2:11 AM

Ignore tests on windows due to limited pluign suport

I don't have commit access, could you commit it for me? ibricchi <ibricchi@student.ethz.ch>

This revision was automatically updated to reflect the committed changes.

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 reopened this revision.Wed, Mar 15, 7:56 AM
This revision is now accepted and ready to land.Wed, Mar 15, 7:56 AM
IBricchi updated this revision to Diff 505492.Wed, Mar 15, 7:56 AM

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?

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!

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

This revision was automatically updated to reflect the committed changes.