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

[WIP] Implement a "place unification" MIR optimization (aka source/destination propagation aka NRVO). #47954

Closed
wants to merge 9 commits into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 1, 2018

nothing to see here (PR open for testing purposes)

note to self: DO NOT MERGE without inspecting all FIXME / HACK comments

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@eddyb
Copy link
Member Author

eddyb commented Feb 1, 2018

r? @nikomatsakis

@bors try

@bors
Copy link
Contributor

bors commented Feb 1, 2018

⌛ Trying commit f88212c with merge 7a4d4c6c9a94179431514d0494125a3339f25718...

@bors
Copy link
Contributor

bors commented Feb 2, 2018

💔 Test failed - status-travis

@Mark-Simulacrum
Copy link
Member

[01:17:24] �[1m�[31merror: internal compiler error�(B�[m�[1m: librustc_trans/mir/block.rs:838: can't directly store to unaligned value�(B�[m
[01:17:24]   �(B�[m�[1m�[34m--> �(B�[m/cargo/registry/src/github.com-1ecc6299db9ec823/failure-0.1.1/src/error.rs:31:5�(B�[m
[01:17:24]    �(B�[m�[1m�[34m|�(B�[m
[01:17:24] �[1m�[34m31�(B�[m �(B�[m�[1m�[34m| �(B�[m�[1m�[31m/�(B�[m �(B�[m    fn from(failure: F) -> Error {�(B�[m
[01:17:24] �[1m�[34m32�(B�[m �(B�[m�[1m�[34m| �(B�[m�[1m�[31m|�(B�[m �(B�[m        let inner: Inner<F> = {�(B�[m
[01:17:24] �[1m�[34m33�(B�[m �(B�[m�[1m�[34m| �(B�[m�[1m�[31m|�(B�[m �(B�[m            let backtrace = if failure.backtrace().is_none() {�(B�[m
[01:17:24] �[1m�[34m34�(B�[m �(B�[m�[1m�[34m| �(B�[m�[1m�[31m|�(B�[m �(B�[m                Backtrace::new()�(B�[m
[01:17:24] �[1m�[34m...�(B�[m  �(B�[m�[1m�[31m|�(B�[m
[01:17:24] �[1m�[34m38�(B�[m �(B�[m�[1m�[34m| �(B�[m�[1m�[31m|�(B�[m �(B�[m        Error { inner: Box::new(inner) }�(B�[m
[01:17:24] �[1m�[34m39�(B�[m �(B�[m�[1m�[34m| �(B�[m�[1m�[31m|�(B�[m �(B�[m    }�(B�[m
[01:17:24]    �(B�[m�[1m�[34m| �(B�[m�[1m�[31m|_____^�(B�[m
[01:17:24] 
[01:17:24] thread 'rustc' panicked at 'Box<Any>', librustc_errors/lib.rs:482:9
[01:17:24] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:17:24] 
[01:17:24] note: the compiler unexpectedly panicked. this is a bug.
[01:17:24] 
[01:17:24] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:17:24] 
[01:17:24] note: rustc 1.25.0-nightly (7a4d4c6c9 2018-02-01) running on x86_64-unknown-linux-gnu
[01:17:24] 
[01:17:24] �[m�[m�[31m�[1merror:�[m Could not compile `cargo`.
[01:17:24] 

@eddyb
Copy link
Member Author

eddyb commented Feb 2, 2018

@bors try

@bors
Copy link
Contributor

bors commented Feb 2, 2018

⌛ Trying commit 401ced3 with merge 51345ce...

bors added a commit that referenced this pull request Feb 2, 2018
[WIP] Implement a "place unification" MIR optimization (aka source/destination propagation).

*nothing to see here* (PR open for testing purposes)

note to self: **DO NOT MERGE** without inspecting all `FIXME` / `HACK` comments
@bors
Copy link
Contributor

bors commented Feb 2, 2018

💔 Test failed - status-travis

@eddyb
Copy link
Member Author

eddyb commented Feb 2, 2018

Looks like banning #[repr(packed)] structs is trickier than I thought.

@shepmaster shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 3, 2018
@bors
Copy link
Contributor

bors commented Feb 3, 2018

☔ The latest upstream changes (presumably #47962) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster
Copy link
Member

shepmaster commented Feb 9, 2018

nothing to see here

👁 We See All 👁

Like the fact that triage notices this has been open for a week, has some CI failures, and some merge conflicts. Any hopes of fixing those issues in the near future?

@eddyb
Copy link
Member Author

eddyb commented Feb 9, 2018

@shepmaster I can close while I'm not using this (I just need to remember to reopen before pushing).

@eddyb eddyb closed this Feb 9, 2018
@eddyb eddyb reopened this Feb 13, 2018
@eddyb eddyb force-pushed the copy-elision branch 2 times, most recently from aee57ce to 44afedf Compare February 13, 2018 13:20
@eddyb
Copy link
Member Author

eddyb commented Feb 13, 2018

@bors try

@bors
Copy link
Contributor

bors commented Feb 13, 2018

⌛ Trying commit 44afedf with merge e8aeb68...

bors added a commit that referenced this pull request Feb 13, 2018
[WIP] Implement a "place unification" MIR optimization (aka source/destination propagation).

*nothing to see here* (PR open for testing purposes)

note to self: **DO NOT MERGE** without inspecting all `FIXME` / `HACK` comments
@bors
Copy link
Contributor

bors commented Feb 13, 2018

💔 Test failed - status-travis

@bors
Copy link
Contributor

bors commented Feb 17, 2018

☔ The latest upstream changes (presumably #47926) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Mar 1, 2018

☀️ Test successful - status-travis
State: approved= try=True

@Mark-Simulacrum
Copy link
Member

Perf started.

@eddyb
Copy link
Member Author

eddyb commented Mar 2, 2018

Latest perf results look much better, worse regression is inflate still but now only a bit under 30x.

@pietroalbini
Copy link
Member

Ping from triage! Do you still need this @eddyb?

@nikomatsakis
Copy link
Contributor

I'm going to go ahead and close this -- @eddyb feel free to re-open. I'm assuming we don't want to merge? (Title is WIP..)

@MSxDOS
Copy link

MSxDOS commented Jan 5, 2020

@eddyb Have you made any progress with this?

@eddyb eddyb reopened this Jan 16, 2020
@eddyb
Copy link
Member Author

eddyb commented Jan 16, 2020

@MSxDOS I may have found a way to make this cheap enough to always run it.
I'll probably get back to it in the coming months.

@mark-i-m
Copy link
Member

mark-i-m commented Mar 5, 2020

Any updates?

@Dylan-DPC-zz
Copy link

Closing this due to inactivity

@ecstatic-morse ecstatic-morse mentioned this pull request May 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 20, 2020
Implement a generic Destination Propagation optimization on MIR

This takes the work that was originally started by @eddyb in rust-lang#47954, and then explored by me in rust-lang#71003, and implements it in a general (ie. not limited to acyclic CFGs) and dataflow-driven way (so that no additional infrastructure in rustc is needed).

The pass is configured to run at `mir-opt-level=2` and higher only. To enable it by default, some followup work on it is still needed:
* Performance needs to be evaluated. I did some light optimization work and tested against `tuple-stress`, which caused trouble in my last attempt, but didn't go much in depth here.
  * We can also enable the pass only at `opt-level=2` and higher, if it is too slow to run in debug mode, but fine when optimizations run anyways.
* Debuginfo needs to be fixed after locals are merged. I did not look into what is required for this.
* Live ranges of locals (aka `StorageLive` and `StorageDead`) are currently deleted. We either need to decide that this is fine, or if not, merge the variable's live ranges (or remove these statements entirely – rust-lang#68622).

Some benchmarks of the pass were done in rust-lang#72635.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2020
Implement a generic Destination Propagation optimization on MIR

This takes the work that was originally started by `@eddyb` in rust-lang#47954, and then explored by me in rust-lang#71003, and implements it in a general (ie. not limited to acyclic CFGs) and dataflow-driven way (so that no additional infrastructure in rustc is needed).

The pass is configured to run at `mir-opt-level=2` and higher only. To enable it by default, some followup work on it is still needed:
* Performance needs to be evaluated. I did some light optimization work and tested against `tuple-stress`, which caused trouble in my last attempt, but didn't go much in depth here.
  * We can also enable the pass only at `opt-level=2` and higher, if it is too slow to run in debug mode, but fine when optimizations run anyways.
* Debuginfo needs to be fixed after locals are merged. I did not look into what is required for this.
* Live ranges of locals (aka `StorageLive` and `StorageDead`) are currently deleted. We either need to decide that this is fine, or if not, merge the variable's live ranges (or remove these statements entirely – rust-lang#68622).

Some benchmarks of the pass were done in rust-lang#72635.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet