generate fewer basic blocks for variant switches #33999
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
cc @dotdash @nagisa -- this is just a minimal fix to address the O(n^2) blowup. The idea was to keep IR the same but not force the creation of one basic block per variant. On IRC, scott mentioned that if you have an enum with 52 variants, we used to generate 2800 basic blocks, but now we generate 212 (when it meets the Any clever ideas how to test this? :) |
| @@ -266,6 +266,7 @@ enum TestKind<'tcx> { | |||
| // test the branches of enum | |||
| Switch { | |||
| adt_def: AdtDef<'tcx>, | |||
| variants: Vec<bool>, | |||
There was a problem hiding this comment.
maybe a good idea to add a comment explaining what this vector represents, when it is true, etc
There was a problem hiding this comment.
Consider using a BitVector from librustc_data_structures.
|
Looks good! I left some minor nits that you could correct. |
| (0..num_enum_variants).map(|_| self.cfg.start_new_block()) | ||
| .collect(); | ||
| .collect() | ||
| }; |
There was a problem hiding this comment.
I think I'd find something like this easier to parse:
let mut otherwise_block = None;
let mut otherwise = || { if otherwise_block.is_none() { otherwise_block = self.cfg.start_new_block()}; otherwise_block.unwrap() };
let target_blocks: Vec<_> = variants.iter().map(|&matched| {
if matched {
self.cfg.start_new_block()
} else {
otherwise()
}
}).collect();
debug!("enum variants: {}, variants.len(): {}, otherwise_block: {:?}", num_enum_variants, variants.len(), otherwise_block);There was a problem hiding this comment.
So I was going to suggest this:
let mut otherwise_block = None;
let target_blocks: Vec<_> = variants.iter().map(|&matched| {
if matched {
self.cfg.start_new_block()
} else {
otherwise_block.unwrap_or_else(|| {
let b = self.cfg.start_new_block();
otherwise_block = Some(b);
b
})
}
}).collect();but I figured "meh". In any case, this has the advantage of passing borrowck, I think :)
|
AFAICT this only removes the extra blocks, it doesn't actually reduce the number of switch arms, there's still one per variant. Additionally, for auto-generated code, you might still get matches that have a bunch of arms that all have no code in them. Similar (but not identical) to what the derive framework did. This patch would not change that at all, it still needs SimplifyCfg and the trans hack. So it's IMHO a bit premature to undo any of those changes. |
|
In that case its not “fixes”, but a “cc”, I think. |
yes
yes
yes, and I wouldn't expect MIR build to notice or optimize this case, would you?
I don't really consider the trans change a hack -- it seems like it's just a more efficient translation from the MIR to the LLVM. So I certainly wouldn't undo it. If we change the MIR to make 'otherwise' a more explicit notion, we could undo it and use that. Seems like an orthogonal thing to me though (it would build on this patch). And it wouldn't be as general as what we have now, given that optimization may create opportunities where you have multiple arms directed to the same block that build does not know about. |
I agree, though I'm not sure if there is a bug left to fix after this lands. |
clarify comments and panic message
No, that is what SimplifyCfg and other passes should handle, i.e. I was arguing that there are uses left for what we currently have, even after the changes in this PR.
I called it a hack only in the way that I agree with @nagisa that it should ultimately become a MIR pass and not be handled just-in-time during trans. IOW I was just arguing for not removing these things yet, because this is not the real replacement yet. I didn't mean to say that we should try to do it all during the build phase. |
|
@bors r+ |
|
📌 Commit 79bf586 has been approved by |
Seems good. :) I think ultimately I'd probably prefer to lower |
|
@bors r+ |
|
📌 Commit d4551ec has been approved by |
|
⌛ Testing commit d4551ec with merge 29b65ff... |
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
@bors retry |
generate fewer basic blocks for variant switches CC #33567 Adds a new field to TestKind::Switch that tracks the variants that are actually matched against. The other candidates target a common "otherwise" block.
|
💔 Test failed - auto-win-gnu-64-nopt-t |
|
cc @alexcrichton weird failure @bors retry |
|
⌛ Testing commit d4551ec with merge f8916da... |
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
@bors: retry On Sun, Jun 5, 2016 at 12:02 AM, bors notifications@github.com wrote:
|
|
⌛ Testing commit d4551ec with merge 22b36c7... |
generate fewer basic blocks for variant switches CC #33567 Adds a new field to TestKind::Switch that tracks the variants that are actually matched against. The other candidates target a common "otherwise" block.
CC #33567
Adds a new field to TestKind::Switch that tracks the variants that are actually matched against. The other candidates target a common "otherwise" block.