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

eskarn · 2021-11-22T09:12:59Z

Opening a draft PR to get feedback and start discussion on this feature. There is already a codegen option passes which allow giving a list of LLVM pass names, however we currently can't use a LLVM pass plugin (as described here : https://llvm.org/docs/WritingAnLLVMPass.html), the only available passes are the LLVM built-in ones.

The proposed modification would be to add another codegen option pass-plugins, which can be set with a list of paths to shared library files. These libraries are loaded using the LLVM function PassPlugin::Load, which calls the expected symbol lvmGetPassPluginInfo, and register the pipeline parsing and optimization callbacks.

An example usage with a single plugin and 3 passes would look like this in the .cargo/config:

rustflags = [
    "-C", "pass-plugins=/tmp/libLLVMPassPlugin",
    "-C", "passes=pass1 pass2 pass3",
]

This would give the same functionality as the opt LLVM tool directly integrated in rust build system.

Additionally, we can also not specify the passes option, and use a plugin which inserts passes in the optimization pipeline, as one could do using clang.

rust-highfive · 2021-11-22T09:13:02Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

Please see the contribution instructions for more information.

nagisa · 2021-11-22T19:06:07Z

How is this different from the flag introduced by #86267?

eskarn · 2021-11-23T08:54:53Z

How is this different from the flag introduced by #86267?

The main difference is using the new LLVM pass manager registration, which is not possible with the current flag.

The flag llvm-plugins allows loading an arbitrary library, and then if the library happens to use the legacy pass manager registration, then its passes can be registered.

The proposed flag would allow loading only libraries which use the new LLVM pass manager format, which means those exposing the symbol lvmGetPassPluginInfo, which gets loaded by PassPlugin::Load.
There is also verification of the LLVM_PLUGIN_API_VERSION, which is declared during the new PM registration.

compiler/rustc_session/src/options.rs

nagisa · 2021-11-23T14:29:24Z

Could the already existing flag be made to work for both oldPM and newPM plugins?

eskarn · 2021-11-23T15:25:45Z

PassPlugin::Load won't load a library without the expected symbol, so we'd need to handle the legacy plugins separately. Keeping only the existing llvm-plugins option, something like this could work:

if the option new_llvm_pass_manager is true, then we skip this code entirely :

rust/compiler/rustc_codegen_llvm/src/llvm_util.rs

Lines 123 to 131 in 22c2d9d

    
           for plugin in &sess.opts.debugging_opts.llvm_plugins { 
        
               let path = Path::new(plugin); 
        
               let res = DynamicLibrary::open(path); 
        
               match res { 
        
                   Ok(_) => debug!("LLVM plugin loaded succesfully {} ({})", path.display(), plugin), 
        
                   Err(e) => bug!("couldn't load plugin: {}", e), 
        
               } 
        
               mem::forget(res); 
        
           }

Then we just keep the proposed modifications in compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp (only using the existing llvm-plugins option instead).

if the option new_llvm_pass_manager is false, we run the code in llvm_util.rs and LLVMRustOptimizeWithNewPassManager won't be called (so we won't try to load the plugins as new PM ones)

The only issue would be when the option isn't specified and the arch is s390x, there is an exception here:

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

Line 409 in 8b09ba6

    
           .unwrap_or_else(|| cgcx.target_arch != "s390x" && llvm_util::get_version() >= (13, 0, 0))

For this specific case, the user of the llvm-plugins would have to know the new PM plugins are not usable because of the exception.

So basically, to use the new PM plugins we'd have to actively set new_llvm_pass_manager to true (and not use the s390x arch).

Would this solution seem acceptable ?

nagisa · 2021-11-23T16:15:18Z

Making it conditional on a flag seems reasonable (oldPM is going away shortly anyway), though I was kind of hoping that PassPlugin::Load would handle plugins for either PM in the first place.

compiler/rustc_codegen_llvm/src/llvm_util.rs

eskarn · 2021-11-24T13:50:25Z

With 0ce67ab389746cd4e135ed15b3c2eb7c32c0d064 and 0ce67ab389746cd4e135ed15b3c2eb7c32c0d064, now we only load new PM plugins from llvm-plugins if new-llvm-pass-manager is set to yes explicitly. Otherwise the behavior is the same as before, loading legacy plugins only.

These changes should address current @bjorn3 and @nagisa feedback.

bjorn3 · 2021-11-24T14:15:28Z

compiler/rustc_codegen_llvm/src/llvm_util.rs

-            Ok(_) => debug!("LLVM plugin loaded succesfully {} ({})", path.display(), plugin),
-            Err(e) => bug!("couldn't load plugin: {}", e),
+    let use_new_llvm_pm_plugin_register =
+        sess.opts.debugging_opts.new_llvm_pass_manager.unwrap_or(false);


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))

}

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 ?

@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 ?

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.

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.

bors · 2021-12-12T20:51:50Z

☔ The latest upstream changes (presumably #90716) made this pull request unmergeable. Please resolve the merge conflicts.

…tration

eskarn · 2021-12-17T14:35:31Z

Resolved conflicts with #90716, re-tested with upstream, and updated the PR title to reflect more what is now being proposed.

nagisa · 2021-12-18T22:41:21Z

Please click the “ready for review” button once you think its good to go.

… llvm_util

nagisa · 2021-12-27T18:08:34Z

@bors r+

bors · 2021-12-27T18:08:36Z

📌 Commit f431df0 has been approved by nagisa

bors · 2021-12-30T02:53:12Z

⌛ Testing commit f431df0 with merge 1b3a5f2...

bors · 2021-12-30T06:06:31Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 1b3a5f2 to master...

rust-timer · 2021-12-30T07:36:38Z

Finished benchmarking commit (1b3a5f2): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

rust-highfive assigned matthewjasper Nov 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2021

bjorn3 reviewed Nov 23, 2021

View reviewed changes

compiler/rustc_session/src/options.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Nov 24, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/llvm_util.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Nov 24, 2021

View reviewed changes

camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 25, 2021

eskarn added 3 commits December 13, 2021 10:40

Add a codegen option to allow loading LLVM pass plugins

97cf461

Use the existing llvm-plugins option for both legacy and new pm regis…

c4f29fa

…tration

Fix conditions for using legacy or new pm plugins

75d1208

eskarn force-pushed the llvm-passes-plugin-support branch from 6e703ce to 75d1208 Compare December 17, 2021 13:44

eskarn changed the title ~~Add a codegen option to allow loading LLVM pass plugins~~ Allow loading LLVM plugins with both legacy and new pass manager Dec 17, 2021

eskarn added 2 commits December 20, 2021 14:49

rustc_codegen_llvm: move should_use_new_llvm_pass_manager function to…

052961b

… llvm_util

Load new pass manager plugins only if the new pm is actually used

f431df0

eskarn marked this pull request as ready for review December 20, 2021 14:42

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 27, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2021

bors merged commit 1b3a5f2 into rust-lang:master Dec 30, 2021

rustbot added this to the 1.59.0 milestone Dec 30, 2021

bors mentioned this pull request Dec 30, 2021

Disable LLVM newPM by default #91190

Closed

cuviper mentioned this pull request Nov 19, 2022

Throw error on failure in loading llvm-plugin #104517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eskarn commented Nov 22, 2021

rust-highfive commented Nov 22, 2021

nagisa commented Nov 22, 2021

eskarn commented Nov 23, 2021 •

edited

Loading

nagisa commented Nov 23, 2021 •

edited

Loading

eskarn commented Nov 23, 2021

nagisa commented Nov 23, 2021 •

edited

Loading

eskarn commented Nov 24, 2021

bjorn3 Nov 24, 2021

eskarn Nov 24, 2021

eskarn Dec 17, 2021

bjorn3 Dec 17, 2021

eskarn Dec 17, 2021 •

edited

Loading

bors commented Dec 12, 2021

eskarn commented Dec 17, 2021

nagisa commented Dec 18, 2021

nagisa commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 30, 2021

bors commented Dec 30, 2021

rust-timer commented Dec 30, 2021

	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))
	}

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

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

Conversation

eskarn commented Nov 22, 2021

rust-highfive commented Nov 22, 2021

nagisa commented Nov 22, 2021

eskarn commented Nov 23, 2021 • edited Loading

nagisa commented Nov 23, 2021 • edited Loading

eskarn commented Nov 23, 2021

nagisa commented Nov 23, 2021 • edited Loading

eskarn commented Nov 24, 2021

bjorn3 Nov 24, 2021

Choose a reason for hiding this comment

eskarn Nov 24, 2021

Choose a reason for hiding this comment

eskarn Dec 17, 2021

Choose a reason for hiding this comment

bjorn3 Dec 17, 2021

Choose a reason for hiding this comment

eskarn Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

bors commented Dec 12, 2021

eskarn commented Dec 17, 2021

nagisa commented Dec 18, 2021

nagisa commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 30, 2021

bors commented Dec 30, 2021

rust-timer commented Dec 30, 2021

eskarn commented Nov 23, 2021 •

edited

Loading

nagisa commented Nov 23, 2021 •

edited

Loading

nagisa commented Nov 23, 2021 •

edited

Loading

eskarn Dec 17, 2021 •

edited

Loading