This is an archive of the discontinued LLVM Phabricator instance.

[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.Mar 8 2023, 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.Mar 10 2023, 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.Mar 10 2023, 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
228–229

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

IBricchi updated this revision to Diff 504630.Mar 13 2023, 6:45 AM
IBricchi marked 5 inline comments as done.

Thanks for the suggestions

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

Thank you for updating the patch. LGTM.

This revision is now accepted and ready to land.Mar 13 2023, 8:38 PM
IBricchi updated this revision to Diff 504988.Mar 14 2023, 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.Mar 15 2023, 7:56 AM
This revision is now accepted and ready to land.Mar 15 2023, 7:56 AM
IBricchi updated this revision to Diff 505492.Mar 15 2023, 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.

This test fails when I build and run with the following:

cmake ../llvm -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON -DLLVM_ENABLE_PLUGINS=ON -DBUILD_SHARED_LIBS=OFF -DLLVM_ENABLE_PROJECTS=clang
ninja unittests/Analysis/AnalysisTests
./unittests/Analysis/AnalysisTests --gtest_filter=PluginInlineOrderTest.NoInlineFoo

I get the following failure:

$ ./unittests/Analysis/AnalysisTests --gtest_filter=PluginInlineOrderTest.NoInlineFoo
Note: Google Test filter = PluginInlineOrderTest.NoInlineFoo
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PluginInlineOrderTest
[ RUN      ] PluginInlineOrderTest.NoInlineFoo
/ssd/upstream/llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp:48: Failure
Value of: !!Plugin
  Actual: false
Expected: true
Plugin path: /ssd/upstream/build2/unittests/Analysis/InlineOrderPlugin.so
[  FAILED  ] PluginInlineOrderTest.NoInlineFoo (4 ms)
[----------] 1 test from PluginInlineOrderTest (4 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (4 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] PluginInlineOrderTest.NoInlineFoo

 1 FAILED TEST

The error returned by CI.setupPlugin() is InlineOrderPlugin.so: undefined symbol: _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyE".

This symbol is llvm::AllAnalysesOn<llvm::Module>::SetKey defined in LLVMCore. It is present in the unit test executable but is a local symbol:

$ readelf -s -W ./unittests/Analysis/AnalysisTests | rg _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyE
 41264: 0000000001c79738     8 OBJECT  LOCAL  DEFAULT   29 _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyE

Therefore it is not resolved by the dlopen.

This comment was removed by ttheodor.