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.

Sign up for GitHub

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

Jump to bottom

smoke-test for async fn with mir-opt-level=0 #71444

Merged
merged 2 commits into from
Apr 28, 2020
Merged

smoke-test for async fn with mir-opt-level=0 #71444

merged 2 commits into from
Apr 28, 2020

Conversation

RalfJung
Copy link
Member

MIR opt levels heavily influence which MIR transformations run, and we barely test non-default opt levels. I am particularly worried about async fn lowering and how it might (not) work when the set of preceding MIR passes changes -- see #70073.

This adds some basic smoke testing, where at least a few async fn run-pass test are ensured to also work with mir-opt-level=0.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2020
@jonas-schievink
Copy link
Contributor

I wouldn't do this for the size tests as those could be affected by other optimizations running. For the smoke tests this seems good though.

@RalfJung
Copy link
Member Author

The size can be affected by MIR optimizations?

@RalfJung RalfJung force-pushed the test-async-no-opt branch from be87eb1 to 9ea5eed Compare April 22, 2020 21:34
@RalfJung
Copy link
Member Author

Okay this does not touch the size tests any more.

@jonas-schievink
Copy link
Contributor

The size can be affected by MIR optimizations?

Potentially yeah, at least in the future when we have some more advanced optimizations.

@RalfJung
Copy link
Member Author

@jonas-schievink any chance you could review this?

jonas-schievink
jonas-schievink approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Looks good. If you want you can also do this for some of the generator tests for some more diversity. But generally r=me

@RalfJung
Copy link
Member Author

@jonas-schievink like so?

@jonas-schievink
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 28, 2020

📌 Commit 3a129df has been approved by jonas-schievink

@bors bors 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 Apr 28, 2020
@bors
Copy link
Contributor

bors commented Apr 28, 2020

⌛ Testing commit 3a129df with merge d7afaa7...

@bors
Copy link
Contributor

bors commented Apr 28, 2020

☀️ Test successful - checks-azure
Approved by: jonas-schievink
Pushing d7afaa7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 28, 2020
@bors bors merged commit d7afaa7 into rust-lang:master Apr 28, 2020
@RalfJung RalfJung deleted the test-async-no-opt branch April 30, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonas-schievink jonas-schievink jonas-schievink approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RalfJung @rust-highfive @jonas-schievink @bors @nikomatsakis