-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Avoid lowering code under dead SwitchInt targets #121421
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Avoid lowering code under dead SwitchInt targets r? `@ghost` Reviving rust-lang#91222 per rust-lang#120848
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
2dcf095
to
642de5d
Compare
This comment has been minimized.
This comment has been minimized.
642de5d
to
6b3d152
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Avoid lowering code under dead SwitchInt targets r? `@ghost` Reviving rust-lang#91222 per rust-lang#120848
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
6b3d152
to
127cb45
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Avoid lowering code under dead SwitchInt targets r? `@ghost` Reviving rust-lang#91222 per rust-lang#120848
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
@bors r+ |
I just checked to see if this PR optimizes out all the panic calls from a compiler-builtins build with debug assertions off and optimizations off, and it looks to me like since #121662 landed, it does not. It isn't entirely clear to me how to fix this. I'll put up a PR in a bit with some suggestions, but I do not like them. |
☀️ Test successful - checks-actions |
Finished benchmarking commit (5a6c1aa): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 674.552s -> 676.304s (0.26%) |
if reachable_blocks.contains(bb) { | ||
fx.codegen_block(bb); | ||
} else { | ||
// This may have references to things we didn't monomorphize, so we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may have references to things we didn't monomorphize
How is that possible? The collector should ensure that we monomorphize everything referenced in the MIR, even in unreachable parts of the MIR.
However it could be interesting to make use of reachable_blocks_in_mono
in the collector, and thus avoid collecting the functions in dead basic blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 I didn't fully update the comment from scottmcm's PR that I stole this from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fixing this in #123272
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw a comment was never sent.
let mut set = BitSet::new_empty(self.basic_blocks.len()); | ||
self.reachable_blocks_in_mono_from(tcx, instance, &mut set, START_BLOCK); | ||
set | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this to traversal.rs
, and refactor the computation to use the standard worklist/visited set algo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in #123272
…try> Only collect mono items from reachable blocks Fixes the wrong commented pointed out in: rust-lang#121421 (comment) Moves the analysis to use the worklist strategy: rust-lang#121421 (comment) Also fixes rust-lang#85836, using the same reachability analysis
…try> Only collect mono items from reachable blocks Fixes the wrong commented pointed out in: rust-lang#121421 (comment) Moves the analysis to use the worklist strategy: rust-lang#121421 (comment) Also fixes rust-lang#85836, using the same reachability analysis
…try> Only collect mono items from reachable blocks Fixes the wrong commented pointed out in: rust-lang#121421 (comment) Moves the analysis to use the worklist strategy: rust-lang#121421 (comment) Also fixes rust-lang#85836, using the same reachability analysis
…try> Only collect mono items from reachable blocks Fixes the wrong commented pointed out in: rust-lang#121421 (comment) Moves the analysis to use the worklist strategy: rust-lang#121421 (comment) Also fixes rust-lang#85836, using the same reachability analysis
…try> Only collect mono items from reachable blocks Fixes the wrong commented pointed out in: rust-lang#121421 (comment) Moves the analysis to use the worklist strategy: rust-lang#121421 (comment) Also fixes rust-lang#85836, using the same reachability analysis
…try> Only collect mono items from reachable blocks Fixes the wrong commented pointed out in: rust-lang#121421 (comment) Moves the analysis to use the worklist strategy: rust-lang#121421 (comment) Also fixes rust-lang#85836, using the same reachability analysis
…jgillot Only collect mono items from reachable blocks Fixes the wrong comment pointed out in: rust-lang#121421 (comment) Moves the analysis to use the worklist strategy: rust-lang#121421 (comment) Also fixes rust-lang#85836, using the same reachability analysis
…jgillot Only collect mono items from reachable blocks Fixes the wrong comment pointed out in: rust-lang#121421 (comment) Moves the analysis to use the worklist strategy: rust-lang#121421 (comment) Also fixes rust-lang#85836, using the same reachability analysis
The objective of this PR is to detect and eliminate code which is guarded by an
if false
, even if thatfalse
is a constant which is not known until monomorphization, or isintrinsics::debug_assertions()
.The effect of this is that we generate no LLVM IR the standard library's unsafe preconditions, when they are compiled in a build where they should be immediately optimized out. This mono-time optimization ensures that builds which disable debug assertions do not grow a linkage requirement against
core
, which compiler-builtins currently needs: #121552This revives the codegen side of #91222 as planned in #120848.