rustc_mir_transform: Make DestinationPropagation stable for queries #119591
Conversation
This file contains hidden or 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
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
Thanks. |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 5, 2024
…ble, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? `@cjgillot` who is reviewer for the highly related PR rust-lang#119252.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 5, 2024
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#118680 (Add support for shell argfiles) - rust-lang#119151 (Hide foreign `#[doc(hidden)]` paths in import suggestions) - rust-lang#119216 (Use diagnostic namespace in stdlib) - rust-lang#119350 (Imply outlives-bounds on lazy type aliases) - rust-lang#119538 (Cleanup error handlers: round 5) - rust-lang#119563 (Check yield terminator's resume type in borrowck) - rust-lang#119589 (cstore: Remove unnecessary locking from `CrateMetadata`) - rust-lang#119591 (rustc_mir_transform: Make DestinationPropagation stable for queries) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 5, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#118680 (Add support for shell argfiles) - rust-lang#119216 (Use diagnostic namespace in stdlib) - rust-lang#119350 (Imply outlives-bounds on lazy type aliases) - rust-lang#119538 (Cleanup error handlers: round 5) - rust-lang#119563 (Check yield terminator's resume type in borrowck) - rust-lang#119577 (Migrate memory overlap check from validator to lint) - rust-lang#119589 (cstore: Remove unnecessary locking from `CrateMetadata`) Failed merges: - rust-lang#119591 (rustc_mir_transform: Make DestinationPropagation stable for queries) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 5, 2024
…ble, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? ```@cjgillot``` who is reviewer for the highly related PR rust-lang#119252.
|
☔ The latest upstream changes (presumably #119621) made this pull request unmergeable. Please resolve the merge conflicts. |
By using FxIndexMap instead of FxHashMap, so that the order of visiting of locals is deterministic. We also need to bless copy_propagation_arg.foo.DestinationPropagation.panic*.diff. Do not review the diff of the diff. Instead look at the diff file before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same.
f603bab to
95eb5bc
Compare
|
@cjgillot Friendly ping to re-approve. There were only conflicts in use declarations. The diff after rebase from what you already approved can most easily be seen with $ git range-diff f603bab^..f603bab 95eb5bc^..95eb5bc
1: f603babd63a ! 1: 95eb5bcb67b rustc_mir_transform: Make DestinationPropagation stable for queries
@@ compiler/rustc_mir_transform/src/dest_prop.rs
-use std::collections::hash_map::{Entry, OccupiedEntry};
-
- use crate::simplify::remove_dead_blocks;
use crate::MirPass;
-use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::{FxIndexMap, IndexEntry, IndexOccupiedEntry};The proper interpretation of that output is: "the only difference between the diff of the approved commit and the diff of the rebased commit is that the line |
|
@bors r=cjgillot |
FxHashMap is actually deterministic given the same HashMap implementation. FxHasher doesn't use randomness unlike the default HashMap hasher ( |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 6, 2024
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#118194 (rustdoc: search for tuples and unit by type with `()`) - rust-lang#118781 (merge core_panic feature into panic_internals) - rust-lang#119486 (pass allow-{dirty,staged} to clippy) - rust-lang#119591 (rustc_mir_transform: Make DestinationPropagation stable for queries) - rust-lang#119595 (Fixed ambiguity in hint.rs) - rust-lang#119624 (rustc_span: More consistent span combination operations) - rust-lang#119653 (compiler: update Fuchsia sanitizer support.) - rust-lang#119655 (Remove ignore-stage1 that was added when changing error count msg) - rust-lang#119661 (Strip lld-wrapper binaries) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 6, 2024
Rollup merge of rust-lang#119591 - Enselic:DestinationPropagation-stable, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? `@cjgillot` who is reviewer for the highly related PR rust-lang#119252.
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.
By using
FxIndexMapinstead ofFxHashMap, so that the order of visiting of locals is deterministic.We also need to bless
copy_propagation_arg.foo.DestinationPropagation.panic*.diff. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix):With this diff (after fix):
and you can see that both before and after the fix, we replace 3 statements with
nops.I find it slightly surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of
FxHashMap, but I guess in can be explained with the predictability of smallFxHashMaps withusize(Local) keys, or something along those lines.This should fix this comment, but I wanted to make a separate PR for this fix for a simpler development and review process.
Part of #84447 which is E-help-wanted.
r? @cjgillot who is reviewer for the highly related PR #119252.