Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] Add codegen option to add passbuilder callback functions #70171

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

wsmoses
Copy link
Member

@wsmoses wsmoses commented Oct 25, 2023

Adding passbuilder callbacks presently can be done one of two ways:

  • a shared library plugin
  • modifying the extensions list (and thus extensions.def)

This prevents the use of such functionality for static builds on build systems that cannot be modified, as well as prevents such functionality from being usable when using llvm/clang as a library.

This creates a third mechanism for adding these callbacks, an explicit list of said callbacks in the CodegenOptions. It's otherwise identical to the plugin approach, except now instead of the list of plugins each being queried for the callback function, this just runs the callback function. This enables use of a clang plugin which can statically add the llvm plugin, without forcing a dsoload for the llvm plugin.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen LTO Link time optimization (regular/full LTO or ThinLTO) labels Oct 25, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 25, 2023

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-lto

@llvm/pr-subscribers-clang-codegen

Author: William Moses (wsmoses)

Changes

Adding passbuilder callbacks presently can be done one of two ways:

  • a shared library plugin
  • modifying the extensions list (and thus extensions.def)

This prevents the use of such functionality for static builds on build systems that cannot be modified, as well as prevents such functionality from being usable when using llvm/clang as a library.

This creates a third mechanism for adding these callbacks, an explicit list of said callbacks. It's otherwise identical to the plugin approach, except now instead of the list of plugins each being queried for the callback function, this just runs the callback function.


Full diff: https://github.com/llvm/llvm-project/pull/70171.diff

4 Files Affected:

  • (modified) clang/lib/CodeGen/BackendUtil.cpp (+3)
  • (modified) llvm/include/llvm/Passes/PassBuilder.h (+5)
  • (modified) llvm/lib/LTO/LTOBackend.cpp (+3)
  • (modified) llvm/tools/opt/NewPMDriver.cpp (+6)
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 70accce456d3c07..8dbe4d942df278d 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -909,6 +909,9 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
 #define HANDLE_EXTENSION(Ext)                                                  \
   get##Ext##PluginInfo().RegisterPassBuilderCallbacks(PB);
 #include "llvm/Support/Extension.def"
+  for (auto PassCallback : ListRegisterPassBuilderCallbacks) {
+    PassCallback(PB);
+  }
 
   // Register the target library analysis directly and give it a customized
   // preset TLI.
diff --git a/llvm/include/llvm/Passes/PassBuilder.h b/llvm/include/llvm/Passes/PassBuilder.h
index 2c7ceda7998eda1..d018dd1e69166f0 100644
--- a/llvm/include/llvm/Passes/PassBuilder.h
+++ b/llvm/include/llvm/Passes/PassBuilder.h
@@ -739,6 +739,11 @@ bool parseAnalysisUtilityPasses(
 
   return false;
 }
+
+//! List of pass builder callbacks to be applied, in addition to those imported
+//! from plugins or LLVM extensions.
+extern SmallVector<std::function<void(PassBuilder &)>>
+    ListRegisterPassBuilderCallbacks;
 }
 
 #endif
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index ccc4276e36dacf0..28aabf8bdebb4d6 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -187,6 +187,9 @@ static void RegisterPassPlugins(ArrayRef<std::string> PassPlugins,
 #define HANDLE_EXTENSION(Ext)                                                  \
   get##Ext##PluginInfo().RegisterPassBuilderCallbacks(PB);
 #include "llvm/Support/Extension.def"
+  for (auto PassCallback : ListRegisterPassBuilderCallbacks) {
+    PassCallback(PB);
+  }
 
   // Load requested pass plugins and let them register pass builder callbacks
   for (auto &PluginFN : PassPlugins) {
diff --git a/llvm/tools/opt/NewPMDriver.cpp b/llvm/tools/opt/NewPMDriver.cpp
index 6ae3f87099afd64..595bc4acddee6c4 100644
--- a/llvm/tools/opt/NewPMDriver.cpp
+++ b/llvm/tools/opt/NewPMDriver.cpp
@@ -431,6 +431,9 @@ bool llvm::runPassPipeline(
 #define HANDLE_EXTENSION(Ext)                                                  \
   get##Ext##PluginInfo().RegisterPassBuilderCallbacks(PB);
 #include "llvm/Support/Extension.def"
+  for (auto PassCallback : ListRegisterPassBuilderCallbacks) {
+    PassCallback(PB);
+  }
 
   // Specially handle the alias analysis manager so that we can register
   // a custom pipeline of AA passes with it.
@@ -546,3 +549,6 @@ void llvm::printPasses(raw_ostream &OS) {
   PassBuilder PB;
   PB.printPassNames(OS);
 }
+
+llvm::SmallVector<std::function<void(PassBuilder &)>>
+    ListRegisterPassBuilderCallbacks;
\ No newline at end of file

@wsmoses wsmoses force-pushed the vectorcallbacks branch 3 times, most recently from 8b0e093 to fca61de Compare October 25, 2023 08:11
@jdoerfert
Copy link
Member

Can you make an example on how this is used now? Is there a way to test this?

@efriedma-quic
Copy link
Collaborator

I don't think a global variable is the right interface for this; we've been trying to move away from global variables because they cause breakage when LLVM is used in multiple contexts in the same process.

Functions that construct pass pipelines that are usable as libraries can take an array of PassPluginLibraryInfo as an argument.

@wsmoses
Copy link
Member Author

wsmoses commented Oct 25, 2023

acks presently can be done one of two ways:

  • a shared library plugin
  • modifying the extensions list (and thus extensions.def)

This prevents the use of such functionality for static builds on build systems that cannot be modified, as well as prevents such functionality from being usable when using llvm/clang as a library.

This creates a third mechanism for adding these callbacks, an explicit list of said callbacks. It's otherwise identical to the plugin approach, except now instead of the list of plugins each being queried for the callback f

Will add later today

@llvmbot llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Oct 26, 2023
@wsmoses
Copy link
Member Author

wsmoses commented Oct 26, 2023

@jdoerfert added the example test

@efriedma-quic I've reworked the mechanism to not create a global variable within LLVM. Here this only applies to clang and enables two things:

  1. Users of clang as a library can add a custom passbuilder callback by adding to codegenoptions
  2. Those who want to modify clang codegen passes can statically link the requisite code (like in the example) to enable their functionality, without forking clang. This still requires a global variable, but here it is limited to the clang driver itself, so it should not create any version conflicts since the clang driver should not load a second clang driver.

@wsmoses wsmoses removed the LTO Link time optimization (regular/full LTO or ThinLTO) label Oct 26, 2023
@wsmoses wsmoses force-pushed the vectorcallbacks branch 3 times, most recently from 227a494 to a49b8e2 Compare October 26, 2023 16:58
@github-actions
Copy link

github-actions bot commented Oct 26, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@efriedma-quic
Copy link
Collaborator

The new option in CodeGenOptions.h seems fine.

If your goal is to statically inject passes into the clang executable, I don't understand why the existing LLVM plugin infrastructure isn't sufficient for your needs. You mention "modifying Extension.def", but that file is generated by the build system based on the plugins found by CMake.

@wsmoses
Copy link
Member Author

wsmoses commented Oct 31, 2023

Let me fork off the clang part off into a separate PR from the codegenoption.

For context, I'm in an environment where I can't modify the LLVM build files (but I can import them), but want to produce the modified clang, without maintaining a separate fork just to add the pass.

@efriedma-quic
Copy link
Collaborator

It should be possible to statically link a pass plugin without modifying any LLVM/clang build files. polly uses this infrastructure. (polly is actually referenced explicitly in a couple places, but only to pull it into the build in the first place; you can do something equivalent with LLVM_EXTERNAL_PROJECTS.)

@wsmoses wsmoses changed the title [PassBuilder] Add a mechanism for adding passbuilder callbacks for static builds [Clang] Add codegen option to add passbuilder callback functions Oct 31, 2023
@wsmoses
Copy link
Member Author

wsmoses commented Oct 31, 2023

@efriedma-quic I've removed the non-options stuff, please review it at your convenience.

Let me see if I can make the other stuff work in a different way.

@wsmoses
Copy link
Member Author

wsmoses commented Nov 3, 2023

bump @efriedma-quic for reviewing this PR

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from an internal chat, this is meant to be used by modifying PassBuilderCallbacks from a statically linked clang plugin. that makes sense to me, could you update the description with this? as for a test, we can add a test similar to PluginsOrder. and then manually verify that statically linking a clang plugin also works for this should be good enough

@@ -906,6 +906,9 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
<< PluginFN << toString(PassPlugin.takeError());
}
}
for (auto PassCallback : CodeGenOpts.PassBuilderCallbacks) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no braces for single line loops

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding the test!

clang/examples/LLVMPrintFunctionNames/CMakeLists.txt Outdated Show resolved Hide resolved
@wsmoses wsmoses merged commit 3c11ac5 into llvm:main Nov 7, 2023
2 of 3 checks passed
@wsmoses wsmoses deleted the vectorcallbacks branch November 7, 2023 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants