Respect -Z verify-llvm-ir and other flags that add extra passes when combined with -C no-prepopulate-passes in the new LLVM Pass Manager. #95893
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…combined with -C no-prepopulate-passes in the new LLVM Pass Manager.
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @nikic |
|
@bors r+ |
|
📌 Commit 84fb481 has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 12, 2022
…s, r=nikic Respect -Z verify-llvm-ir and other flags that add extra passes when combined with -C no-prepopulate-passes in the new LLVM Pass Manager. As part of the switch to the new LLVM Pass Manager the behaviour of flags such as `-Z verify-llvm-ir` (e.g. sanitizer, instrumentation) was modified when combined with `-C no-prepopulate-passes`. With the old PM, rustc was the one manually constructing the pipeline and respected those flags but in the new pass manager, those flags are used to build a list of callbacks that get invoked at certain extension points in the pipeline. Unfortunately, `-C no-prepopulate-passes` would skip building the pipeline altogether meaning we'd never add the corresponding passes. The fix here is to just manually invoke those callbacks as needed. Fixes rust-lang#95874 Demonstrating the current vs fixed behaviour using the bug in rust-lang#95864 ```console $ rustc +nightly asm-miscompile.rs --edition 2021 --emit=llvm-ir -C no-prepopulate-passes -Z verify-llvm-ir $ echo $? 0 $ rustc +stage1 asm-miscompile.rs --edition 2021 --emit=llvm-ir -C no-prepopulate-passes -Z verify-llvm-ir Basic Block in function '_ZN14asm_miscompile3foo28_$u7b$$u7b$closure$u7d$$u7d$17h360e2f7eee1275c5E' does not have terminator! label %bb1 LLVM ERROR: Broken module found, compilation aborted! ```
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (b8f4cb6): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Footnotes |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part of the switch to the new LLVM Pass Manager the behaviour of flags such as
-Z verify-llvm-ir(e.g. sanitizer, instrumentation) was modified when combined with-C no-prepopulate-passes. With the old PM, rustc was the one manually constructing the pipeline and respected those flags but in the new pass manager, those flags are used to build a list of callbacks that get invoked at certain extension points in the pipeline. Unfortunately,-C no-prepopulate-passeswould skip building the pipeline altogether meaning we'd never add the corresponding passes. The fix here is to just manually invoke those callbacks as needed.Fixes #95874
Demonstrating the current vs fixed behaviour using the bug in #95864