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

Allow early otherwise branch on mir-opt-level=2 #82905

Closed

Conversation

spastorino
Copy link
Member

Let's test the performance of making this change, note that mir-opt-level is 2 by default in optimized builds.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2021
@spastorino
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 8, 2021
@bors
Copy link
Contributor

bors commented Mar 8, 2021

⌛ Trying commit f37adef with merge 76689995512a02828b62e1adda15545c90423f80...

@bors
Copy link
Contributor

bors commented Mar 8, 2021

☀️ Try build successful - checks-actions
Build commit: 76689995512a02828b62e1adda15545c90423f80 (76689995512a02828b62e1adda15545c90423f80)

@rust-timer
Copy link
Collaborator

Queued 76689995512a02828b62e1adda15545c90423f80 with parent 1d6b0f6, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (76689995512a02828b62e1adda15545c90423f80): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 8, 2021
@bjorn3
Copy link
Member

bjorn3 commented Mar 9, 2021

Many improvements of up to 0.6%. Some regressions up to 0.5%.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 9, 2021

Any changes to non *-opt runs must either be noise or changes in how this optimization makes rustc faster/slower.

cc @rust-lang/wg-mir-opt I think we have our first candidate for a MIR optimization that we only turn on in release mode.

As to what this optimization does, I think the documentation explains that best:

/// This pass optimizes something like
/// ```text
/// let x: Option<()>;
/// let y: Option<()>;
/// match (x,y) {
/// (Some(_), Some(_)) => {0},
/// _ => {1}
/// }
/// ```
/// into something like
/// ```text
/// let x: Option<()>;
/// let y: Option<()>;
/// let discriminant_x = // get discriminant of x
/// let discriminant_y = // get discriminant of y
/// if discriminant_x != discriminant_y || discriminant_x == None {1} else {0}

@tmiasko
Copy link
Contributor

tmiasko commented Mar 9, 2021

I think the transformation is incorrect for reasons described in #78496 (comment).

@oli-obk
Copy link
Contributor

oli-obk commented Mar 9, 2021

@spastorino please add a check for unsound mir opts to this optimization, referencing the linked issue.

@spastorino
Copy link
Member Author

Closing this one as I've just opened #83277.

@spastorino spastorino closed this Mar 18, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 19, 2021
…und, r=oli-obk

Mark early otherwise optimization unsound

r? `@oli-obk`
cc `@tmiasko`

Related to rust-lang#78496 and rust-lang#82905

Should I also bump this one to level 3 or 4 or given that is unsound it doesn't matter?.
Probably need to adjust some tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants