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

rustc_mir_transform: Make DestinationPropagation stable for queries #119591

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Jan 4, 2024

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):

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 small FxHashMaps with usize (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.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 4, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@cjgillot
Copy link
Contributor

cjgillot commented Jan 4, 2024

Thanks.
@bors r+

@bors
Copy link
Contributor

bors commented Jan 4, 2024

📌 Commit f603bab has been approved by cjgillot

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 Jan 4, 2024
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.
@bors
Copy link
Contributor

bors commented Jan 5, 2024

☔ The latest upstream changes (presumably #119621) made this pull request unmergeable. Please resolve the merge conflicts.

@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 Jan 5, 2024
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.
@Enselic Enselic force-pushed the DestinationPropagation-stable branch from f603bab to 95eb5bc Compare January 5, 2024 20:00
@Enselic
Copy link
Member Author

Enselic commented Jan 5, 2024

@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:

$ 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 use crate::simplify::remove_dead_blocks; was removed". Which makes sense since that line was removed on master by an unrelated commit (namely this).

@tmiasko
Copy link
Contributor

tmiasko commented Jan 5, 2024

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Jan 5, 2024

📌 Commit 95eb5bc has been approved by cjgillot

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 Jan 5, 2024
@bjorn3
Copy link
Member

bjorn3 commented Jan 6, 2024

I find it slightly surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of FxHashMap

FxHashMap is actually deterministic given the same HashMap implementation. FxHasher doesn't use randomness unlike the default HashMap hasher (RandomState).

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
@bors bors merged commit 909f2b6 into rust-lang:master Jan 6, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 6, 2024
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
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants