Skip to content

[mir-opt] Introduce a new flag to enable experimental/unsound mir opts - #76899

Merged
bors merged 2 commits into
rust-lang:masterfrom
wesleywiser:experimental_unsound_mir_opts_flag
Sep 28, 2020
Merged

[mir-opt] Introduce a new flag to enable experimental/unsound mir opts#76899
bors merged 2 commits into
rust-lang:masterfrom
wesleywiser:experimental_unsound_mir_opts_flag

Conversation

@wesleywiser

@wesleywiserwesleywiser commented Sep 19, 2020

Copy link
Copy Markdown
Member

This implements part of rust-lang/compiler-team#319. The exact name of this flag was not decided as part of that MCP and some people expressed that it should include "unsound" in some way.

I've chosen to use enable-experimental-unsound-mir-opts as the name. While long, I don't think that matters too much as really it will only be used by some mir-opt tests. If you object or have a better name, please leave a comment!

r? @oli-obk
cc @rust-lang/wg-mir-opt @RalfJung

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2020
@Aaron1011

Aaron1011 commented Sep 19, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

I would personally prefer unsound-mir-opts:

  • It's easier to type.
  • We could theoretically move a non-experimental optimization pass behind this flag if we discovered it was unsound.
  • I think 'enable' is implied by the presence of the flag itself.

oli-obk
oli-obk reviewed Sep 19, 2020
View reviewed changes
Comment threadsrc/test/mir-opt/inline/inline-any-operand.rs Outdated
oli-obk
oli-obk reviewed Sep 19, 2020
View reviewed changes
Comment threadsrc/test/mir-opt/inline/inline-closure-borrows-arg.rs Outdated
oli-obk
oli-obk reviewed Sep 19, 2020
View reviewed changes
Comment threadsrc/test/mir-opt/inline/inline-trait-method_2.rs Outdated
oli-obk
oli-obk reviewed Sep 19, 2020
View reviewed changes
Comment threadsrc/test/mir-opt/nrvo-simple.rs Outdated
@wesleywiser

wesleywiser commented Sep 19, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@oli-obk I tried to keep our MIR opt tests' output as close to the current ones as possible. Some of these passes run after copy-prop so the effects of it being disabled by default (even under mir-opt-level=2) show up as changes in the tests.

If you think we should not add this flag to those tests and accept the changes instead, I'd be happy to do so.

@oli-obk

oli-obk commented Sep 19, 2020

Copy link
Copy Markdown
Contributor

If you think we should not add this flag to those tests and accept the changes instead, I'd be happy to do so.

Maybe remove the flag again in a separate commit so we keep the changes separate?

@oli-obk

oli-obk commented Sep 19, 2020

Copy link
Copy Markdown
Contributor

With that and the flag renamed to unsound-mir-opts as suggested: r=me

@RalfJung

RalfJung commented Sep 19, 2020 via email

Copy link
Copy Markdown
Member

@wesleywiser

wesleywiser commented Sep 19, 2020

Copy link
Copy Markdown
MemberAuthor

@RalfJung I remember you felt quite strongly the option name should include "unsound". Does unsound-mir-opts work for you?

@tmiasko

tmiasko commented Sep 19, 2020

Copy link
Copy Markdown
Contributor

Is this option intended only for unsound passes or also that are buggy in other ways e.g., ICE? If the latter I would also include inlining pass. Recently when I evaluated mir-opt-level=2 on a different crates, rustc generally ICEd due to normalization issues in inliner or other issues exposed indirectly by inlining. The attempt to enable inlining in #75495 didn't fare any better.

@wesleywiser

wesleywiser commented Sep 19, 2020

Copy link
Copy Markdown
MemberAuthor

Originally, I was thinking this flag could handle both but now I'm thinking we should probably have two. The optimizations that have bugs which result in unsoundness are far worse than the ones that "just" ICE the compiler.

@bors

bors commented Sep 20, 2020

Copy link
Copy Markdown
Collaborator

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@RalfJung

RalfJung commented Sep 20, 2020

Copy link
Copy Markdown
Member

@RalfJung I remember you felt quite strongly the option name should include "unsound". Does unsound-mir-opts work for you?

Yes I like that name. :)

@wesleywiser
wesleywiserforce-pushed the experimental_unsound_mir_opts_flag branch 2 times, most recently from 4f4cc60 to 3498511CompareSeptember 21, 2020 02:11
@wesleywiser

wesleywiser commented Sep 21, 2020

Copy link
Copy Markdown
MemberAuthor

@bors r=oli-obk

@bors

bors commented Sep 21, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 3498511 has been approved by oli-obk

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 22, 2020
@Dylan-DPC
Rollup merge ofrust-lang#76899- wesleywiser:experimental_unsound_mi…
aa2dcb3
…r_opts_flag, r=oli-obk
[mir-opt] Introduce a new flag to enable experimental/unsound mir opts
This implements part of rust-lang/compiler-team#319. The exact name of this flag was not decided as part of that MCP and some people expressed that it should include "unsound" in some way.
I've chosen to use `enable-experimental-unsound-mir-opts` as the name. While long, I don't think that matters too much as really it will only be used by some mir-opt tests. If you object or have a better name, please leave a comment!
r? @oli-obk
cc @rust-lang/wg-mir-opt @RalfJung
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Sep 22, 2020
Closed
@Dylan-DPC-zz

Dylan-DPC-zz commented Sep 23, 2020

Copy link
Copy Markdown

failed in rollup CI

@bors r-

@borsbors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 23, 2020
@pnkfelixpnkfelix mentioned this pull request Sep 24, 2020
Merged
@wesleywiser
wesleywiserforce-pushed the experimental_unsound_mir_opts_flag branch from 3498511 to 2c5f061CompareSeptember 25, 2020 00:50
@wesleywiser

wesleywiser commented Sep 25, 2020

Copy link
Copy Markdown
MemberAuthor

Rebased and resolved the failing test from the rollup.

@bors r=oli-obk

@bors

bors commented Sep 25, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 2c5f061d187b346c7f9e229a53be41723164ec30 has been approved by oli-obk

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 25, 2020
@bors

bors commented Sep 25, 2020

Copy link
Copy Markdown
Collaborator

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@borsbors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 25, 2020
@wesleywiser
wesleywiserforce-pushed the experimental_unsound_mir_opts_flag branch from 2c5f061 to c653af8CompareSeptember 27, 2020 23:29
@wesleywiser

wesleywiser commented Sep 28, 2020

Copy link
Copy Markdown
MemberAuthor

Rebased

@bors r=oli-obk

@bors

bors commented Sep 28, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit c653af8 has been approved by oli-obk

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 28, 2020
@bors

bors commented Sep 28, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit c653af8 with merge d62d3f7...

@bors

bors commented Sep 28, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing d62d3f7 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Sep 28, 2020
@bors
bors merged commit d62d3f7 into rust-lang:masterSep 28, 2020
@rustbotrustbot added this to the 1.48.0 milestone Sep 28, 2020
@wesleywiserwesleywiser mentioned this pull request Sep 29, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obkoli-obkoli-obk left review comments

Assignees

@oli-obkoli-obk

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.48.0

Development

Successfully merging this pull request may close these issues.

9 participants

@wesleywiser@Aaron1011@oli-obk@RalfJung@tmiasko@bors@Dylan-DPC-zz@rust-highfive@rustbot