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 -Zmir-opt-level=3 -Zvalidate-mir #105706

Closed
saethlin opened this issue Dec 14, 2022 · 4 comments · Fixed by #105736
Closed

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

saethlin opened this issue Dec 14, 2022 · 4 comments · Fixed by #105736
Assignees
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-testsuite Area: The testsuite used to check the correctness of rustc E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@saethlin
Copy link
Member

saethlin commented Dec 14, 2022

Because currently it doesn't. -Zmir-opt-level=3 has a handful of problems which are reported elsewhere, but discovering problems with MIR optimizations is often as simple as building the compiler with the optimization enabled and -Zvalidate-mir. So we should do that in CI.

Note that all MIR opt levels are supposed to be sound, and known unsoundness is supposed to be gated behind -Zunsound-mir-opts, which I am not suggesting we test with.


Currently attempting to build the standard library with these flags results in pages of this error:

error: internal compiler error: broken MIR in Item(WithOptConstParam { did: DefId(0:20363 ~ core[b221]::core_simd::masks::mask_impl::{impl#6}::to_bitmask_integer), const_param_did: None }) (after pass DestinationPropagation) at bb2[2]:
                                encountered overlapping memory in `Call` terminator: _0 = <U as ReverseBits>::reverse_bits(move _0, const LANES) -> [return: bb3, unwind: bb6] 
   --> /home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/../../portable-simd/crates/core_simd/src/masks/full_masks.rs:217:13
    |    
217 |             bitmask.reverse_bits(LANES)
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |    
    = note: delayed at compiler/rustc_const_eval/src/transform/validate.rs:779:26

Which looks to me like #105428

@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html E-help-wanted Call for participation: Help is requested to fix this issue. labels Dec 14, 2022
@jyn514
Copy link
Member

jyn514 commented Dec 14, 2022

@saethlin also mentioned to me that some opts are using -Zmir-opt-level=4 as a minimum threshold, it may be helpful to test that as well (maybe in a separate builder).

-Zvalidate-mir does not change the output of the compiler and should be ok to turn on unconditionally for libstd; we might want to be more selective about where we enable it for the compiler itself, though, since it's a lot slower.

We should also have at least one builder where we enable -Zvalidate-mir -Zmir-opt-level=3 for UI tests.

@jyn514 jyn514 added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Dec 14, 2022
@jyn514
Copy link
Member

jyn514 commented Dec 14, 2022

Mentoring instructions:

  1. Add new rust.validate_mir_opts = N option to Config and config.toml.example:
    pub struct Config {
    . This option should pass -Zvalidate-mir -Zmir-opt-level=N unconditionally for libstd. Test it works locally with build --stage 1 std, you should see the ICE @saethlin posted above.
  2. Enable that new option on all builders around here:
    RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set rust.codegen-units-std=1"
    . You can test that works by opening a PR (please put r? @jyn514 in the description) and making sure it shows the same ICE.
  3. Assuming that works, we can figure out whether we want to enable this for rustc as well on some builders; maybe we should have separate validate-mir-opts-std and validate-mir-opts-rustc configs? And validate-mir-opts-testsuite for turning it on in compiletest?

@jyn514 jyn514 added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 14, 2022
@chenyukang
Copy link
Member

@rustbot claim

@chenyukang
Copy link
Member

Reproduced it on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-testsuite Area: The testsuite used to check the correctness of rustc E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants