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

Test that the compiler/library builds with validate-mir #105736

Merged
merged 3 commits into from
Feb 26, 2023

Conversation

chenyukang
Copy link
Member

Fixes #105706

@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2022

r? @jyn514

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

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 15, 2022
@chenyukang chenyukang force-pushed the yukang/add-mir-opt-level-testing branch 2 times, most recently from 0bc4dc1 to 6098888 Compare December 15, 2022 08:46
@saethlin
Copy link
Member

@jyn514 I think this is good to go now, the fix just merged

@chenyukang chenyukang force-pushed the yukang/add-mir-opt-level-testing branch 2 times, most recently from a37296b to cdfdf77 Compare December 19, 2022 03:16
@chenyukang
Copy link
Member Author

@saethlin
Seems some UI are still failing, I rebased with master.

@rust-log-analyzer

This comment has been minimized.

@saethlin
Copy link
Member

Ah, those are not going to pass as a result of any other PR, you need to do something here to get their output back to what they're supposed to be. Probably we want to default all test suites to MIR opt level 3, except for the mir-opt suite.

You'll probably also run into breakage in codegen or debuginfo tests from changing their MIR opt level, that might resolve itself when #105577 lands.

@jyn514
Copy link
Member

jyn514 commented Dec 25, 2022

Ah, those are not going to pass as a result of any other PR, you need to do something here to get their output back to what they're supposed to be. Probably we want to default all test suites to MIR opt level 3, except for the mir-opt suite.

That implies we need to not set mir-opt-level for the mir-opt test suite, right? So this change probably needs to happen at least partly in compiletest, not just bootstrap.

@chenyukang
Copy link
Member Author

Ah, those are not going to pass as a result of any other PR, you need to do something here to get their output back to what they're supposed to be. Probably we want to default all test suites to MIR opt level 3, except for the mir-opt suite.

That implies we need to not set mir-opt-level for the mir-opt test suite, right? So this change probably needs to happen at least partly in compiletest, not just bootstrap.

Is it this line make compiletest run with different options?

https://github.com/rust-lang/rust/pull/105736/files#diff-79e3e43fa59f1f75c3e925ab874d736286eb2ac7e1e08f3b7f05bae1a67f082fR67

when in mir-opt mode, we set mir-opt-level here:
https://github.com/rust-lang/rust/blob/e3961864075eaa9e855e5eec6b4f148029684539/src/tools/compiletest/src/runtest.rs#LL1952

@chenyukang chenyukang force-pushed the yukang/add-mir-opt-level-testing branch 2 times, most recently from 8e0f54e to ef7db07 Compare December 28, 2022 14:50
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@chenyukang chenyukang force-pushed the yukang/add-mir-opt-level-testing branch from 99d823f to 9beb57a Compare December 29, 2022 06:44
@chenyukang
Copy link
Member Author

chenyukang commented Dec 29, 2022

@saethlin
do you know why we add mir-opt-level for stage1 std will make mir-opt fail?

x test --stage 1 src/test/mir-opt

If so, we can not enable it for stage1 std.
I plan to enable it for test cases(except for those mir-opt testcases).

@rust-log-analyzer

This comment has been minimized.

@saethlin
Copy link
Member

saethlin commented Dec 29, 2022

Ah yeah I should have expected this. Let me do some experimentation (based on your branch) locally, I'll get back to you within a day.

@chenyukang
Copy link
Member Author

Ah yeah I should have expected this. Let me do some experimentation (based on your branch) locally, I'll get back to you within a day.

Thanks, use this to reproduce it locally:

./configure --set rust.validate-mir-opts=3
x test --stage 1 src/test/mir-opt

@saethlin
Copy link
Member

I think we're in a sticky situation here. What I originally envisioned was a fully separate step in CI where we only build and do not run any tests, we would just rely on the MIR validation. If that's possible to do, I think it is the best way to dodge the situation I will ramble on about now.

The problem you're running into is we compile the standard library ahead of time with a separate set of flags than are specified in mir-opt and codegen tests (and I suppose various other UI tests as well). So changing the build settings for the standard library will break tests when standard library functions get inlined into the tests. One other way around this is to always build the distributed standard library at the higher MIR opt level, then bless the tests. I'm told cranelift uses a higher MIR opt level for the standard library already, though based on gestures broadly I don't think I would advise doing that at this moment. (This also drags in the question of what happens to users of build-std, do they get a standard library with less optimizations because the default release build is mir opt level 2? That seems bad.)

@jyn514
Copy link
Member

jyn514 commented Dec 30, 2022

I think for now we should only land this for the standard library itself; I think that would have avoided most of the issues @saethlin mentioned in the past. We can worry about doing it for the test suite and compiler later.

@jyn514
Copy link
Member

jyn514 commented Dec 30, 2022

Oh I see, even doing it for just the standard library causes issues. That's ... very unfortunate.

We don't run any tests on mingw-check - maybe we can add this new check to only that builder? Then the different MIR output shouldn't cause any tests to fail.

I'm told cranelift uses a higher MIR opt level for the standard library already, though based on gestures broadly I don't think I would advise doing that at this moment. (This also drags in the question of what happens to users of build-std, do they get a standard library with less optimizations because the default release build is mir opt level 2? That seems bad.)

I don't think we should penalize all users just because some users wouldn't benefit. As long as we're not actively hurting build-std people, I don't think that should be a reason to avoid adding more optimizations for dist builds (Mark-Simulacrum has already said a few times people should use the official builds if they want to match performance characteristics anyway). But if you're not confident all the opt-level=2 opts are sound, we can avoid turning them on for now.

@jyn514 jyn514 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 30, 2022
@chenyukang chenyukang force-pushed the yukang/add-mir-opt-level-testing branch from 31129c7 to 1302069 Compare February 24, 2023 22:20
@chenyukang chenyukang force-pushed the yukang/add-mir-opt-level-testing branch from 12ca5a4 to 15813cf Compare February 24, 2023 22:40
@jyn514
Copy link
Member

jyn514 commented Feb 24, 2023

This looks good to me! can you just verify by looking at the CI logs that validate-mir-opts is actually being passed to configure? github actions is being annoying and won't let me look at the full logs until the run finishes 😞 but it will look something like configure: build.configure-args := ['--set', 'rust.validate-mir-opts']

@jyn514
Copy link
Member

jyn514 commented Feb 24, 2023

@bors
Copy link
Contributor

bors commented Feb 24, 2023

📌 Commit 7d3457bb1220df1afac72eab41806c82cb67d250 has been approved by jyn514

It is now in the queue for this repository.

@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 Feb 24, 2023
@jyn514
Copy link
Member

jyn514 commented Feb 24, 2023

oh sorry, can you fix #105736 (comment) first?

@bors r- delegate=chenyukang

(please use @bors r=jyn514 when you've fixed it :)

@bors bors 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 Feb 24, 2023
@bors
Copy link
Contributor

bors commented Feb 24, 2023

✌️ @chenyukang can now approve this pull request

@bors
Copy link
Contributor

bors commented Feb 24, 2023

📌 Commit 7d3457bb1220df1afac72eab41806c82cb67d250 has been approved by `jyn514``

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2023
@jyn514
Copy link
Member

jyn514 commented Feb 24, 2023

@bors r-

@bors bors 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 Feb 24, 2023
@chenyukang chenyukang force-pushed the yukang/add-mir-opt-level-testing branch from 7d3457b to 001bcee Compare February 25, 2023 03:11
@chenyukang
Copy link
Member Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Feb 25, 2023

📌 Commit 001bcee has been approved by jyn514

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2023
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#105736 (Test that the compiler/library builds with validate-mir)
 - rust-lang#107291 ([breaking change] Remove a rustdoc back compat warning)
 - rust-lang#107675 (Implement -Zlink-directives=yes/no)
 - rust-lang#107848 (Split `x setup` sub-actions to CLI arguments)
 - rust-lang#107911 (Add check for invalid #[macro_export] arguments)
 - rust-lang#108229 ([107049] Recognise top level keys in config.toml.example)
 - rust-lang#108333 (Make object bound candidates sound in the new trait solver)

Failed merges:

 - rust-lang#108337 (hir-analysis: make a helpful note)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b90a385 into rust-lang:master Feb 26, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test that the compiler/library builds with -Zmir-opt-level=3 -Zvalidate-mir
7 participants