Skip to content

Allow loading LLVM plugins with both legacy and new pass manager #91125

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.

Sign up for GitHub

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

Merged
bors merged 5 commits into from
Dec 30, 2021
Merged

Allow loading LLVM plugins with both legacy and new pass manager #91125

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97cf461
Add a codegen option to allow loading LLVM pass plugins
eskarn Nov 19, 2021
c4f29fa
Use the existing llvm-plugins option for both legacy and new pm regis…
eskarn Nov 24, 2021
75d1208
Fix conditions for using legacy or new pm plugins
eskarn Nov 24, 2021
052961b
rustc_codegen_llvm: move should_use_new_llvm_pass_manager function to…
eskarn Dec 20, 2021
f431df0
Load new pass manager plugins only if the new pm is actually used
eskarn Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions compiler/rustc_codegen_llvm/src/back/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,8 @@ pub(crate) unsafe fn optimize_with_new_llvm_pass_manager(

let extra_passes = config.passes.join(",");

let llvm_plugins = config.llvm_plugins.join(",");

// FIXME: NewPM doesn't provide a facility to pass custom InlineParams.
// We would have to add upstream support for this first, before we can support
// config.inline_threshold and our more aggressive default thresholds.
Expand Down Expand Up @@ -499,6 +501,8 @@ pub(crate) unsafe fn optimize_with_new_llvm_pass_manager(
selfprofile_after_pass_callback,
extra_passes.as_ptr().cast(),
extra_passes.len(),
llvm_plugins.as_ptr().cast(),
llvm_plugins.len(),
);
result.into_result().map_err(|()| llvm_err(diag_handler, "failed to run LLVM passes"))
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_codegen_llvm/src/llvm/ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2304,6 +2304,8 @@ extern "C" {
end_callback: SelfProfileAfterPassCallback,
ExtraPasses: *const c_char,
ExtraPassesLen: size_t,
LLVMPlugins: *const c_char,
LLVMPluginsLen: size_t,
) -> LLVMRustResult;
pub fn LLVMRustPrintModule(
M: &'a Module,
Expand Down
22 changes: 14 additions & 8 deletions compiler/rustc_codegen_llvm/src/llvm_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,20 @@ unsafe fn configure_llvm(sess: &Session) {

llvm::LLVMInitializePasses();

// Register LLVM plugins by loading them into the compiler process.
for plugin in &sess.opts.debugging_opts.llvm_plugins {
let lib = Library::new(plugin).unwrap_or_else(|e| bug!("couldn't load plugin: {}", e));
debug!("LLVM plugin loaded successfully {:?} ({})", lib, plugin);

// Intentionally leak the dynamic library. We can't ever unload it
// since the library can make things that will live arbitrarily long.
mem::forget(lib);
let use_new_llvm_pm_plugin_register =
sess.opts.debugging_opts.new_llvm_pass_manager.unwrap_or(false);
Copy link
Member

@bjorn3 bjorn3 Nov 24, 2021

Choose a reason for hiding this comment

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

If new_llvm_pass_manager returns None, the new pass manager is used when the llvm version is at least 13 and the target is not s390x. See

rust/compiler/rustc_codegen_llvm/src/back/write.rs

Lines 397 to 410 in 8b09ba6

pub(crate) fn should_use_new_llvm_pass_manager(
cgcx: &CodegenContext<LlvmCodegenBackend>,
config: &ModuleConfig,
) -> bool {
// The new pass manager is enabled by default for LLVM >= 13.
// This matches Clang, which also enables it since Clang 13.
// FIXME: There are some perf issues with the new pass manager
// when targeting s390x, so it is temporarily disabled for that
// arch, see https://github.com/rust-lang/rust/issues/89609
config
.new_llvm_pass_manager
.unwrap_or_else(|| cgcx.target_arch != "s390x" && llvm_util::get_version() >= (13, 0, 0))
}

Copy link
Contributor Author

@eskarn eskarn Nov 24, 2021

Choose a reason for hiding this comment

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

Yes that's what i was mentioning at the end of #91125 (comment).

What i'm proposing is to condition the new PM plugin loading only on the explicit new_llvm_pass_manager=yes and not the actual usage of the new PM later. Since there are exceptions and a default value hardcoded, we can't trust the option to reflect reality.

If we do want to condition on the actual usage of the new PM then i see two potential issues:

  • current users of llvm-plugins when updating to a llvm13 based rust will trigger PassPlugin::Load on their legacy plugins since the new PM is enabled by default. They will need to explicitly set new_llvm_pass_manager=false
  • i'm not sure how to use should_use_new_llvm_pass_manager in llvm_util.rs, maybe just move it to llvm_util.rs to make it more widely available ?

Copy link
Contributor Author

@eskarn eskarn Dec 17, 2021

Choose a reason for hiding this comment

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

@bjorn3 do you think the current code is acceptable regarding the explicit usage of new_llvm_pass_manager or i should
propose the alternative described in my last reply ?

Copy link
Member

@bjorn3 bjorn3 Dec 17, 2021

Choose a reason for hiding this comment

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

Sorry for the late reply. Missed your previous comment.

current users of llvm-plugins when updating to a llvm13 based rust will trigger PassPlugin::Load on their legacy plugins since the new PM is enabled by default. They will need to explicitly set new_llvm_pass_manager=false

At least in that case you will get an error message rather than silently ignoring it, right?

i'm not sure how to use should_use_new_llvm_pass_manager in llvm_util.rs, maybe just move it to llvm_util.rs to make it more widely available ?

Makes sense I think.

do you think the current code is acceptable regarding the explicit usage of new_llvm_pass_manager or i should
propose the alternative described in my last reply ?

I think using should_use_new_llvm_pass_manager is better. I don't strongly object to the current code though.

Copy link
Contributor Author

@eskarn eskarn Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the late reply. Missed your previous comment.

No problem, i should have pinged you earlier but i was busy on other tasks anyway.

current users of llvm-plugins when updating to a llvm13 based rust will trigger PassPlugin::Load on their legacy plugins since the new PM is enabled by default. They will need to explicitly set new_llvm_pass_manager=false

At least in that case you will get an error message rather than silently ignoring it, right?

Indeed, in that case cargo build fails with an error and i can see original LLVM error message directly in cargo non verbose logs like this: Plugin entry point not found in '/tmp/libLLVMPlugin.so'. Is this a legacy plugin?

i'm not sure how to use should_use_new_llvm_pass_manager in llvm_util.rs, maybe just move it to llvm_util.rs to make it more widely available ?

Makes sense I think.

Ok let's try it.

do you think the current code is acceptable regarding the explicit usage of new_llvm_pass_manager or i should
propose the alternative described in my last reply ?

I think using should_use_new_llvm_pass_manager is better. I don't strongly object to the current code though.

Ok thank you for your feedback, i'll try doing it this way and see how that goes.


// Use the legacy pm registration if the new_llvm_pass_manager option isn't explicitly enabled
if !use_new_llvm_pm_plugin_register {
// Register LLVM plugins by loading them into the compiler process.
for plugin in &sess.opts.debugging_opts.llvm_plugins {
let lib = Library::new(plugin).unwrap_or_else(|e| bug!("couldn't load plugin: {}", e));
debug!("LLVM plugin loaded successfully {:?} ({})", lib, plugin);

// Intentionally leak the dynamic library. We can't ever unload it
// since the library can make things that will live arbitrarily long.
mem::forget(lib);
}
}

rustc_llvm::initialize_available_targets();
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_codegen_ssa/src/back/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ pub struct ModuleConfig {
pub inline_threshold: Option<u32>,
pub new_llvm_pass_manager: Option<bool>,
pub emit_lifetime_markers: bool,
pub llvm_plugins: Vec<String>,
}

impl ModuleConfig {
Expand Down Expand Up @@ -260,6 +261,11 @@ impl ModuleConfig {
inline_threshold: sess.opts.cg.inline_threshold,
new_llvm_pass_manager: sess.opts.debugging_opts.new_llvm_pass_manager,
emit_lifetime_markers: sess.emit_lifetime_markers(),
llvm_plugins: if sess.opts.debugging_opts.new_llvm_pass_manager.unwrap_or(false) {
if_regular!(sess.opts.debugging_opts.llvm_plugins.clone(), vec![])
} else {
vec![]
},
}
}

Expand Down
18 changes: 17 additions & 1 deletion compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Object/IRObjectFile.h"
#include "llvm/Passes/PassBuilder.h"
#include "llvm/Passes/PassPlugin.h"
#include "llvm/Passes/StandardInstrumentations.h"
#include "llvm/Support/CBindingWrapping.h"
#include "llvm/Support/FileSystem.h"
Expand Down Expand Up @@ -753,7 +754,8 @@ LLVMRustOptimizeWithNewPassManager(
void* LlvmSelfProfiler,
LLVMRustSelfProfileBeforePassCallback BeforePassCallback,
LLVMRustSelfProfileAfterPassCallback AfterPassCallback,
const char *ExtraPasses, size_t ExtraPassesLen) {
const char *ExtraPasses, size_t ExtraPassesLen,
const char *LLVMPlugins, size_t LLVMPluginsLen) {
Module *TheModule = unwrap(ModuleRef);
TargetMachine *TM = unwrap(TMRef);
OptimizationLevel OptLevel = fromRust(OptLevelRust);
Expand Down Expand Up @@ -924,6 +926,20 @@ LLVMRustOptimizeWithNewPassManager(
}
}

if (LLVMPluginsLen) {
auto PluginsStr = StringRef(LLVMPlugins, LLVMPluginsLen);
SmallVector<StringRef> Plugins;
PluginsStr.split(Plugins, ',', -1, false);
for (auto PluginPath: Plugins) {
auto Plugin = PassPlugin::Load(PluginPath.str());
if (!Plugin) {
LLVMRustSetLastError(("Failed to load pass plugin" + PluginPath.str()).c_str());
continue;
}
Plugin->registerPassBuilderCallbacks(PB);
}
}

#if LLVM_VERSION_GE(13, 0)
ModulePassManager MPM;
#else
Expand Down