Make validate_mir ensure the final MIR for all bodies
#128612
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
hi @rust-lang/wg-mir-opt anyone got strong feelings abt this? |
compiler-errors
force-pushed
the
validate-mir-opt-mir
branch
from
bfb8ab6
to
f48fe22
Compare
|
For the record, I have a fix for #128601 but want to put up better test that doesn't need the polymorphize flag to trigger it. |
|
Ah, let me just use the instance mir instead of opt mir, since we have some const body owners. |
This comment has been minimized.
This comment has been minimized.
|
this is like |
compiler-errors
force-pushed
the
validate-mir-opt-mir
branch
2 times, most recently
from
74dace7
to
470ada2
Compare
compiler-errors
changed the title
validate_mir ensure optimized MIR for all bodiesvalidate_mir ensure the final MIR for all bodies
davidtwco
approved these changes
Aug 6, 2024
compiler-errors
force-pushed
the
validate-mir-opt-mir
branch
from
470ada2
to
1e07c19
Compare
|
@bors r=davidtwco rollup |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Aug 8, 2024
…ir, r=davidtwco Make `validate_mir` ensure the final MIR for all bodies A lot of the crashes tests use `-Zpolymorphize` or `-Zdump-mir` for their side effect of computing the `optimized_mir` for all bodies, which will uncover bugs with late MIR passes like the inliner. I don't like having all these tests depend on `-Zpolymorphize` (or other hacky ways) for no reason, so this PR extends the `-Zvalidate-mir` flag to ensure `optimized_mir`/`mir_for_ctfe` for all body owners during the analysis phase. Two thoughts: 1. This could be moved later in the compilation pipeline I guess? I don't really think it matters, though. 1. This could alternatively be expressed using a new flag, though I don't necessarily see much value in separating these. For example, rust-lang#128171 could have used this flag, in the `tests/ui/polymorphization/inline-incorrect-early-bound.rs`. r? mir
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 8, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#128306 (Update NonNull::align_offset quarantees) - rust-lang#128612 (Make `validate_mir` ensure the final MIR for all bodies) - rust-lang#128648 (Add regression test) - rust-lang#128791 (Don't implement `AsyncFn` for `FnDef`/`FnPtr` that wouldnt implement `Fn`) - rust-lang#128795 (Update E0517 message to reflect RFC 2195.) - rust-lang#128825 (rm `declared_features` field in resolver) - rust-lang#128826 (Only suggest `#[allow]` for `--warn` and `--deny` lint level flags) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Aug 8, 2024
…ir, r=davidtwco Make `validate_mir` ensure the final MIR for all bodies A lot of the crashes tests use `-Zpolymorphize` or `-Zdump-mir` for their side effect of computing the `optimized_mir` for all bodies, which will uncover bugs with late MIR passes like the inliner. I don't like having all these tests depend on `-Zpolymorphize` (or other hacky ways) for no reason, so this PR extends the `-Zvalidate-mir` flag to ensure `optimized_mir`/`mir_for_ctfe` for all body owners during the analysis phase. Two thoughts: 1. This could be moved later in the compilation pipeline I guess? I don't really think it matters, though. 1. This could alternatively be expressed using a new flag, though I don't necessarily see much value in separating these. For example, rust-lang#128171 could have used this flag, in the `tests/ui/polymorphization/inline-incorrect-early-bound.rs`. r? mir
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 8, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#128306 (Update NonNull::align_offset quarantees) - rust-lang#128612 (Make `validate_mir` ensure the final MIR for all bodies) - rust-lang#128648 (Add regression test) - rust-lang#128749 (Mark `{f32,f64}::{next_up,next_down,midpoint}` inline) - rust-lang#128795 (Update E0517 message to reflect RFC 2195.) - rust-lang#128825 (rm `declared_features` field in resolver) - rust-lang#128826 (Only suggest `#[allow]` for `--warn` and `--deny` lint level flags) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 8, 2024
Rollup merge of rust-lang#128612 - compiler-errors:validate-mir-opt-mir, r=davidtwco Make `validate_mir` ensure the final MIR for all bodies A lot of the crashes tests use `-Zpolymorphize` or `-Zdump-mir` for their side effect of computing the `optimized_mir` for all bodies, which will uncover bugs with late MIR passes like the inliner. I don't like having all these tests depend on `-Zpolymorphize` (or other hacky ways) for no reason, so this PR extends the `-Zvalidate-mir` flag to ensure `optimized_mir`/`mir_for_ctfe` for all body owners during the analysis phase. Two thoughts: 1. This could be moved later in the compilation pipeline I guess? I don't really think it matters, though. 1. This could alternatively be expressed using a new flag, though I don't necessarily see much value in separating these. For example, rust-lang#128171 could have used this flag, in the `tests/ui/polymorphization/inline-incorrect-early-bound.rs`. r? mir
|
somehow github does not recognize this as merged, closing manually.. |
|
@matthiaskrgr: it's because bors merged an old version :( Since you merge a lot of PRs and are likely to catch this: When you close PRs that GH doesn't mention, can you check that the version that is pushed is the one that shows up in the bors approval? #128612 (comment) |
|
oh wut? :O usually it would show the commits being added after the rollup being mentioned in the pr history? |
|
oh I see, there are some comments in this pr that were not in the rollup... |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Aug 8, 2024
…ir, r=matthiaskrgr Add comment that bors did not see pushed before it merged In rust-lang#128612, bors merged 470ada2 instead of 1e07c19. This means it dropped a useful comment I added, and a stage rename that is more descriptive.
|
☔ The latest upstream changes (presumably #128835) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 9, 2024
Rollup merge of rust-lang#128851 - compiler-errors:validate-mir-opt-mir, r=matthiaskrgr Add comment that bors did not see pushed before it merged In rust-lang#128612, bors merged 470ada2 instead of 1e07c19. This means it dropped a useful comment I added, and a stage rename that is more descriptive.
This was referenced Aug 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A lot of the crashes tests use
-Zpolymorphizeor-Zdump-mirfor their side effect of computing theoptimized_mirfor all bodies, which will uncover bugs with late MIR passes like the inliner. I don't like having all these tests depend on-Zpolymorphize(or other hacky ways) for no reason, so this PR extends the-Zvalidate-mirflag to ensureoptimized_mir/mir_for_ctfefor all body owners during the analysis phase.Two thoughts:
For example, #128171 could have used this flag, in the
tests/ui/polymorphization/inline-incorrect-early-bound.rs.r? mir