Skip to content
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

[MIR trans] Optimize trans for biased switches #33566

Merged
merged 1 commit into from
May 14, 2016

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented May 11, 2016

Currently, all switches in MIR are exhausitive, meaning that we can have
a lot of arms that all go to the same basic block, the extreme case
being an if-let expression which results in just 2 possible cases, be
might end up with hundreds of arms for large enums.

To improve this situation and give LLVM less code to chew on, we can
detect whether there's a pre-dominant target basic block in a switch
and then promote this to be the default target, not translating the
corresponding arms at all.

In combination with #33544 this makes unoptimized MIR trans of
nickel.rs as fast as using old trans and greatly improves the times for
optimized builds, which are only 30-40% slower instead of ~300%.

cc #33111

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

// If a single target basic blocks is predominant, promote that to be the
// default case for the switch instruction to reduce the size of the generated
// code. This is especially helpful in cases like an if-let on a huge enum.
Some((&&bb, &c)) if c as f32 / targets.len() as f32 >= 0.05 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

c > targets.len()/2

// instruction so LLVM knows that
let unreachable_blk = self.unreachable_block();
let switch = bcx.switch(discr, unreachable_blk.llbb, targets.len());
let mut bb_hist = FnvHashMap();
Copy link
Member

@nagisa nagisa May 11, 2016

Choose a reason for hiding this comment

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

You should add a FIXME here to check for the validity of such transformation. While currently the otherwise branch is always unreachable (and contains a panic), this might not be the case after a bunch of optimisation passes are added to the MIR thus making this transformation invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note.

Copy link
Contributor

Choose a reason for hiding this comment

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

TerminatorKind::Switch can't have an otherwise branch in any case.

Copy link
Member

Choose a reason for hiding this comment

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

There's also the assert_eq!(adt_def.variants.len(), targets.len()); which should in theory make sure we've covered all the cases.

Currently, all switches in MIR are exhausitive, meaning that we can have
a lot of arms that all go to the same basic block, the extreme case
being an if-let expression which results in just 2 possible cases, be
might end up with hundreds of arms for large enums.

To improve this situation and give LLVM less code to chew on, we can
detect whether there's a pre-dominant target basic block in a switch
and then promote this to be the default target, not translating the
corresponding arms at all.

In combination with rust-lang#33544 this makes unoptimized MIR trans of
nickel.rs as fast as using old trans and greatly improves the times for
optimized builds, which are only 30-40% slower instead of ~300%.

cc rust-lang#33111
@nagisa
Copy link
Member

nagisa commented May 11, 2016

I guess this is fine to land. I’d still prefer it to become a proper MIR transformation down the line but this can be done later.

@bors r+

@bors
Copy link
Contributor

bors commented May 11, 2016

📌 Commit 49b2cdf has been approved by nagisa

Manishearth added a commit to Manishearth/rust that referenced this pull request May 12, 2016
[MIR trans] Optimize trans for biased switches

Currently, all switches in MIR are exhausitive, meaning that we can have
a lot of arms that all go to the same basic block, the extreme case
being an if-let expression which results in just 2 possible cases, be
might end up with hundreds of arms for large enums.

To improve this situation and give LLVM less code to chew on, we can
detect whether there's a pre-dominant target basic block in a switch
and then promote this to be the default target, not translating the
corresponding arms at all.

In combination with rust-lang#33544 this makes unoptimized MIR trans of
nickel.rs as fast as using old trans and greatly improves the times for
optimized builds, which are only 30-40% slower instead of ~300%.

cc rust-lang#33111
eddyb added a commit to eddyb/rust that referenced this pull request May 12, 2016
[MIR trans] Optimize trans for biased switches

Currently, all switches in MIR are exhausitive, meaning that we can have
a lot of arms that all go to the same basic block, the extreme case
being an if-let expression which results in just 2 possible cases, be
might end up with hundreds of arms for large enums.

To improve this situation and give LLVM less code to chew on, we can
detect whether there's a pre-dominant target basic block in a switch
and then promote this to be the default target, not translating the
corresponding arms at all.

In combination with rust-lang#33544 this makes unoptimized MIR trans of
nickel.rs as fast as using old trans and greatly improves the times for
optimized builds, which are only 30-40% slower instead of ~300%.

cc rust-lang#33111
bors added a commit that referenced this pull request May 12, 2016
eddyb added a commit to eddyb/rust that referenced this pull request May 13, 2016
[MIR trans] Optimize trans for biased switches

Currently, all switches in MIR are exhausitive, meaning that we can have
a lot of arms that all go to the same basic block, the extreme case
being an if-let expression which results in just 2 possible cases, be
might end up with hundreds of arms for large enums.

To improve this situation and give LLVM less code to chew on, we can
detect whether there's a pre-dominant target basic block in a switch
and then promote this to be the default target, not translating the
corresponding arms at all.

In combination with rust-lang#33544 this makes unoptimized MIR trans of
nickel.rs as fast as using old trans and greatly improves the times for
optimized builds, which are only 30-40% slower instead of ~300%.

cc rust-lang#33111
Manishearth added a commit to Manishearth/rust that referenced this pull request May 14, 2016
[MIR trans] Optimize trans for biased switches

Currently, all switches in MIR are exhausitive, meaning that we can have
a lot of arms that all go to the same basic block, the extreme case
being an if-let expression which results in just 2 possible cases, be
might end up with hundreds of arms for large enums.

To improve this situation and give LLVM less code to chew on, we can
detect whether there's a pre-dominant target basic block in a switch
and then promote this to be the default target, not translating the
corresponding arms at all.

In combination with rust-lang#33544 this makes unoptimized MIR trans of
nickel.rs as fast as using old trans and greatly improves the times for
optimized builds, which are only 30-40% slower instead of ~300%.

cc rust-lang#33111
bors added a commit that referenced this pull request May 14, 2016
Rollup of 9 pull requests

- Successful merges: #33544, #33552, #33554, #33555, #33560, #33566, #33572, #33574, #33576
- Failed merges:
@bors bors merged commit 49b2cdf into rust-lang:master May 14, 2016
@dotdash dotdash deleted the biased_switch branch May 17, 2016 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants