This is an archive of the discontinued LLVM Phabricator instance.

[InlineAdvisor] Allow loading advisors as plugins
ClosedPublic

Authored by IBricchi on Dec 8 2022, 9:19 AM.

Details

Summary

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

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

Diff Detail

Event Timeline

IBricchi created this revision.Dec 8 2022, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 9:19 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
IBricchi requested review of this revision.Dec 8 2022, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 9:19 AM
IBricchi set the repository for this revision to rG LLVM Github Monorepo.Dec 8 2022, 9:19 AM
mtrofin added a subscriber: phosek.Dec 8 2022, 9:55 AM

lgtm, but could you please get someone to look at the cmakelists stuff, I'm not sure if that's the most canonical llvm way to do that... maybe @phosek, but AFAIK he may be out for the remainder of the year

We will see if @phosek has time, but for context we imitated https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt for the cmake changes.

We will see if @phosek has time, but for context we imitated https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt for the cmake changes.

OK, but why did you need to make everything LLVM_OPTIONAL_SOURCES and not just the plugin-loading test?

The reason we need to do this is because otherwise we fail this check: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/LLVMProcessSources.cmake#L105

The check is making sure that there aren't any dead files in a folder, and it does this by making sure all files are used by all targets. However in our case we have two targets and each one uses a subset of the files so we have to use the LLVM_OPTIONAL_SOURCES to tell CMAKE that the files not being used by a given target aren't dead.

The reason we need to do this is because otherwise we fail this check: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/LLVMProcessSources.cmake#L105

The check is making sure that there aren't any dead files in a folder, and it does this by making sure all files are used by all targets. However in our case we have two targets and each one uses a subset of the files so we have to use the LLVM_OPTIONAL_SOURCES to tell CMAKE that the files not being used by a given target aren't dead.

Right, but my question is why does everything get made LLVM_OPTIONAL_SOURCES and not just the subset that's not used on the specific target?

Right I see, well there are two targets, the first is the unit tests which uses all the files except "InlineAdvisorPlugin.cpp", and then there's the target for the inlineAdvisorPlugin which only uses "InlineAdvisorPlugin.cpp", so for the first target we need "InlineAdvisorPlugin.cpp" in the optional source, and for the second target we need everything else.

Right I see, well there are two targets, the first is the unit tests which uses all the files except "InlineAdvisorPlugin.cpp", and then there's the target for the inlineAdvisorPlugin which only uses "InlineAdvisorPlugin.cpp", so for the first target we need "InlineAdvisorPlugin.cpp" in the optional source, and for the second target we need everything else.

Huh. Why would you not want to run the other tests when you run the plugin advisor? If it's because those tests don't work on that target, I'd recommend making that factoring first - i.e. just the cmakelists change(s) that exclude unwanted tests from that target.

IBricchi updated this revision to Diff 483323.Dec 15 2022, 1:43 PM

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

OK, but that doesn't explain why it'd want to do the opposite for the test plugin target.

Also, my suggestion was to do that change - i.e. the change that makes conditional compilation of certain files (except for the to-be-introduced plugin unit test) in a separate patch, which would have to land first. This is so that the concerns are separated: one thing is about having a different build behavior on certain platforms. The other is adding the new unit test.

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

OK, but that doesn't explain why it'd want to do the opposite for the test plugin target.

Without setting LLVM_OPTIONAL_SOURCES before add_llvm_library(InlineAdvisorPlugin MODULE BUILDTREE_ONLY InlineAdvisorPlugin.cpp) we get the following error:

CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AliasAnalysisTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)


CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AliasSetTrackerTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)


CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AssumeBundleQueriesTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)

...
#Continues listing all other source files in the directory

The issue is that the add_llvm_library expects that all source files in the current directory are used by its target; it does this via llvm_check_source_file_list by scanning the current directory and checking the target's sources. llvm_check_source_file_list ignores files in LLVM_OPTIONAL_SOURCES and is meant for CMakeLists.txt that define multiple targets (other unit tests that define such plugins also do this). That's why we need to set all other files as optional sources before adding the plugin module.

Also, my suggestion was to do that change - i.e. the change that makes conditional compilation of certain files (except for the to-be-introduced plugin unit test) in a separate patch, which would have to land first. This is so that the concerns are separated: one thing is about having a different build behavior on certain platforms. The other is adding the new unit test.

The only conditionally built target is the plugin module, everything else is built normally as before. Note that LLVM_OPTIONAL_SOURCES is set after add_llvm_unittest_with_input_files(AnalysisTests ..., so the original unit test is not affected.

We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target

OK, but that doesn't explain why it'd want to do the opposite for the test plugin target.

Without setting LLVM_OPTIONAL_SOURCES before add_llvm_library(InlineAdvisorPlugin MODULE BUILDTREE_ONLY InlineAdvisorPlugin.cpp) we get the following error:

CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AliasAnalysisTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)


CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AliasSetTrackerTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)


CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
  Found unknown source file AssumeBundleQueriesTest.cpp

  Please update
  /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
  cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
  cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
  unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)

...
#Continues listing all other source files in the directory

The issue is that the add_llvm_library expects that all source files in the current directory are used by its target; it does this via llvm_check_source_file_list by scanning the current directory and checking the target's sources. llvm_check_source_file_list ignores files in LLVM_OPTIONAL_SOURCES and is meant for CMakeLists.txt that define multiple targets (other unit tests that define such plugins also do this). That's why we need to set all other files as optional sources before adding the plugin module.

My main question is why you needed to have a target that has only the plugin - I think the answer is that it's because you're building 2 targets now from the same cmake lists, and the second one, the InlineAdvisorPlugin, should just have the plugin, correct?

How about:

  • define a variable, e.g. ANALYSIS_TEST_SOURCES, that lists all the sources at lines 26-66
  • then: add_llvm_unittest_with_input_files(AnalysisTests ${ANALYSIS_TEST_SOURCES}) and then you don't need to re-list all the sources again (and, thus, avoid maintenance issues), and just concatenate to LLVM_OPTIONAL_SOURCES the whole ANALYSIS_TEST_SOURCE list and please add a comment as to why this happens (the reason above: so you can build the second, stand-alone plugin)

I'd also suggest this second bit happened *only* when you build the plugin, that would make things more clear - so under the line 119.

I think that would both keep the file maintainable - when tests need to be added, they just need to be added to the ANALYSIS_TEST_SOURCE - and clarify that the expansion of LLVM_OPTIONAL_SOURCES only happens if and only if you need to build the extra plugin standalone library.

wdyt?

Also, my suggestion was to do that change - i.e. the change that makes conditional compilation of certain files (except for the to-be-introduced plugin unit test) in a separate patch, which would have to land first. This is so that the concerns are separated: one thing is about having a different build behavior on certain platforms. The other is adding the new unit test.

The only conditionally built target is the plugin module, everything else is built normally as before. Note that LLVM_OPTIONAL_SOURCES is set after add_llvm_unittest_with_input_files(AnalysisTests ..., so the original unit test is not affected.

llvm/unittests/Analysis/CMakeLists.txt
10

Use include/llvm/Config/llvm-config.h.cmake instead define the preprocessor macro.

IBricchi updated this revision to Diff 483571.Dec 16 2022, 9:57 AM

I think your suggestions make sense, I've updated the cmake file to address those points.

I think your suggestions make sense, I've updated the cmake file to address those points.

Thanks, looks good - just the thing about llvm-config.h.cmake, it's the canonical way to "convert" cmake flags to macro directives - so you wouldn't need to do :

if (LLVM_ENABLE_PLUGINS)
  add_definitions(-DLLVM_ENABLE_PLUGINS)
endif()

and, added bonus, others in the future that would want to #ifdef LLVM_ENABLE_PLUGINS would be able to do so.

llvm/unittests/Analysis/CMakeLists.txt
68

see the early comment about using llvm-configlh.cmake here

IBricchi updated this revision to Diff 483602.Dec 16 2022, 10:52 AM

I must have missed that comment in my previous pass, I've now updated the llvm-config.h.cmake file

mtrofin accepted this revision.Dec 16 2022, 11:25 AM
This revision is now accepted and ready to land.Dec 16 2022, 11:25 AM

I don't have commit access to LLVM, would it be possible for you to commit it for me @mtrofin, or is there a different process I need to take?

I don't have commit access to LLVM, would it be possible for you to commit it for me @mtrofin, or is there a different process I need to take?

I'll do it.

mtrofin added a comment.EditedDec 16 2022, 12:32 PM

I don't have commit access to LLVM, would it be possible for you to commit it for me @mtrofin, or is there a different process I need to take?

I'll do it.

@IBricchi , you need to rebase the patch out off origin/main (i.e. git branch -vv should say origin/main for your branch). Can you please do that and update it - then I can arc patch properly. Thanks!

OK, that worked, but looks like the author wasn't set in your commit. What should I set it to? (i.e. name and email)

IBricchi updated this revision to Diff 483657.EditedDec 16 2022, 1:38 PM

Updated.
Author: ibricchi
Email: ibricchi@student.ethz.ch

Updated.
Author: ibricchi
Email: ibricchi@student.ethz.ch

perfect - btw, when you rebased, the llvm-config.h change got lost, can you please fix? thanks!

IBricchi updated this revision to Diff 483668.Dec 16 2022, 2:10 PM

My bad, should be fixed now

My bad, should be fixed now

Thanks

Have you tried building from scratch - i.e. rm -rf your build dir, and start from cmake - because I'm getting this:

/other/llvm-project/llvm/include/llvm/IR/Attributes.h:90:14: fatal error: 'llvm/IR/Attributes.inc' file not found
    #include "llvm/IR/Attributes.inc"

I tried and I couldn't replicate what you described. Would you be able to say what cmake options you're using so I can try those exactly

mtrofin added a comment.EditedDec 16 2022, 3:17 PM

I tried and I couldn't replicate what you described. Would you be able to say what cmake options you're using so I can try those exactly

sorted out - error on my side. sorry about the false alarm.

...or so I thought. My local fix was not specifying LLVM_BINUTILS_INCDIR, but it seems that was just hiding the problem. I attached to the revert commit message a few build bot links that should give sufficient context for a repro.

This revision was landed with ongoing or failed builds.Dec 16 2022, 4:01 PM
This revision was automatically updated to reflect the committed changes.
IBricchi updated this revision to Diff 483742.Dec 17 2022, 4:35 AM

I managed to re-create the error and I think it was caused by the order in which things were being built. I added a dependancy for the plugin so that Attributes.inc is ready by the time the plugin comes around to being built.

IBricchi reopened this revision.Dec 17 2022, 4:36 AM
This revision is now accepted and ready to land.Dec 17 2022, 4:36 AM
mtrofin added inline comments.Dec 17 2022, 10:35 AM
llvm/unittests/Analysis/CMakeLists.txt
60

note: I got rid of the EXTRA_TESTS since having ANALYSIS_TEST_SOURCES removes the need for an extra list: e can just append to the latter.

99

Ack.

Hi @IBricchi, the compiler error is still occurring (as of 1a9dec0dda0064cd0035d70b6534a65856b7a212).
To reproduce create a clean build directory (configure using ninja) and run ninja unittests/Analysis/CMakeFiles/InlineAdvisorPlugin.dir/InlineAdvisorPlugin.cpp.o.

IBricchi updated this revision to Diff 483782.Dec 17 2022, 3:13 PM
IBricchi updated this revision to Diff 483783.Dec 17 2022, 3:21 PM
thakis added a subscriber: thakis.Dec 18 2022, 4:29 AM

The CMake setup here is pretty unusual. LLVM uses one target per cmake file almost everywhere, and many of the macros expect that. Maybe the plugin could be in a plugin/ subdir of the test and that might make things easier? clang/lib/Analysis/plugins/ has examples of that (which are loaded via lit).

(The two targets in unittests/Passes/CMakeLists.txt are _also_ unusual.)

Thanks, looks good - just the thing about llvm-config.h.cmake, it's the canonical way to "convert" cmake flags to macro directives - so you wouldn't need to do :

That's not fully true. For things that are internal to llvm, that place is config.h I think. llvm-config.h is for places outside of LLVM.

(The two targets in unittests/Passes/CMakeLists.txt are _also_ unusual.)(The two targets in unittests/Passes/CMakeLists.txt are _also_ unusual.)

(And it's kind of not a great place since it means toggling a single cmake option means rebuilding all files that read any cmake setting, but it's the current setup.)

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

By the way, was there an RFC somewhere that discussed why this is a good thing for the project? (I have no opinion on this myself, but it clearly comes with a cost, so it'd be good if people working in this domain overall agreed that this is a good direction.)

thakis added inline comments.Dec 18 2022, 4:39 AM
llvm/unittests/Analysis/CMakeLists.txt
10

Also, why did 7d2c1150d31bb remove the comment together with the define? The comment seems useful independent of the define.

thakis added inline comments.Dec 18 2022, 5:39 AM
llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp
65

This test is added unconditionally and doesn't check LLVM_ENABLE_PLUGINS as far as I can tell. How does this work with plugins disabled?

Also, I'm getting Could not load library '...InlineAdvisorPlugin.so': ...InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm27PluginInlineAdvisorAnalysis17HasBeenRegisteredE when running the test. That's probably because the plugin includes the file that declares that file-level static but then doesn't have a definition of it in the linkage unit (?)

thakis added inline comments.Dec 18 2022, 6:10 AM
llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp
65

The check for LLVM_ENABLE_PLUGINS is below on line 262. Sorry for the noise.

mtrofin added inline comments.Dec 18 2022, 7:31 AM
llvm/unittests/Analysis/CMakeLists.txt
10

As the define becomes available part of llvm-config.h (or config.h - separate issue), there's nothing left to comment on in the cmake, and there's a comment to the same effect in PluginsTest.cpp.

Also, I'm getting Could not load library '...InlineAdvisorPlugin.so': ...InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm27PluginInlineAdvisorAnalysis17HasBeenRegisteredE when running the test. That's probably because the plugin includes the file that declares that file-level static but then doesn't have a definition of it in the linkage unit (?)

This was due to me not linking AnalysisTests with -rdynamic.

Things work now over here.

The "this is unusual" comment remains :)

Also, I'm getting Could not load library '...InlineAdvisorPlugin.so': ...InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm27PluginInlineAdvisorAnalysis17HasBeenRegisteredE when running the test. That's probably because the plugin includes the file that declares that file-level static but then doesn't have a definition of it in the linkage unit (?)

This was due to me not linking AnalysisTests with -rdynamic.

Things work now over here.

The "this is unusual" comment remains :)

Can we help @IBricchi with a recommendation as to how to author a test - for example, perhaps having a subdirectory for the test plugin?

The reason we went with this general structure for the cmake file is that the other unit tests that implement similar functionality implement it like this:

https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt

I am still seeing this linking error when building with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS: /unittests/Analysis/InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyEStack.

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

By the way, was there an RFC somewhere that discussed why this is a good thing for the project? (I have no opinion on this myself, but it clearly comes with a cost, so it'd be good if people working in this domain overall agreed that this is a good direction.)

BTW, was this ever answered?

I am still seeing this linking error when building with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS: /unittests/Analysis/InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyEStack.

I can't seem to get this to happen, and the build-bots on main don't seem to encounter it either.

Would you be able to tell me how to replicate this?

Adds the ability to load InlineAdvisors as plugins. This allows developing and distributing inlining heuristics outside of tree.

By the way, was there an RFC somewhere that discussed why this is a good thing for the project? (I have no opinion on this myself, but it clearly comes with a cost, so it'd be good if people working in this domain overall agreed that this is a good direction.)

BTW, was this ever answered?

Usefulness: In our group (and this applies to others as well) we have been working on inlining for a while. However our work is experimental/research, and it makes little sense to try to upstream it directly on LLVM. his creates a barrier for sharing it with the community so that it can be used and evaluated. LLVM already had the correct abstraction to solve our problem, the InliningAdvisor; all that was missing was what this patch to enable the ability to externally provide one. This is greatly reduces the barrier for sharing inlining (heuristic) related work, not only for our group, but anyone who's working on that.

Cost: the only runtime cost is a boolean check, whether a PluginAdvisor has been registered, which should not even be measurable.