Skip to content

Commit

Permalink
Rollup merge of rust-lang#119591 - Enselic:DestinationPropagation-sta…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
matthiaskrgr authored Jan 5, 2024
2 parents 6a47d09 + f603bab commit 6548f08
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 34 deletions.
1 change: 1 addition & 0 deletions compiler/rustc_data_structures/src/fx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub type StdEntry<'a, K, V> = std::collections::hash_map::Entry<'a, K, V>;
pub type FxIndexMap<K, V> = indexmap::IndexMap<K, V, BuildHasherDefault<FxHasher>>;
pub type FxIndexSet<V> = indexmap::IndexSet<V, BuildHasherDefault<FxHasher>>;
pub type IndexEntry<'a, K, V> = indexmap::map::Entry<'a, K, V>;
pub type IndexOccupiedEntry<'a, K, V> = indexmap::map::OccupiedEntry<'a, K, V>;

#[macro_export]
macro_rules! define_id_collections {
Expand Down
30 changes: 14 additions & 16 deletions compiler/rustc_mir_transform/src/dest_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,9 @@
//! [attempt 2]: https://github.com/rust-lang/rust/pull/71003
//! [attempt 3]: https://github.com/rust-lang/rust/pull/72632
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};
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
use rustc_middle::mir::HasLocalDecls;
Expand Down Expand Up @@ -212,7 +210,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation {
let mut merged_locals: BitSet<Local> = BitSet::new_empty(body.local_decls.len());

// This is the set of merges we will apply this round. It is a subset of the candidates.
let mut merges = FxHashMap::default();
let mut merges = FxIndexMap::default();

for (src, candidates) in candidates.c.iter() {
if merged_locals.contains(*src) {
Expand Down Expand Up @@ -257,8 +255,8 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation {
/// frequently. Everything with a `&'alloc` lifetime points into here.
#[derive(Default)]
struct Allocations {
candidates: FxHashMap<Local, Vec<Local>>,
candidates_reverse: FxHashMap<Local, Vec<Local>>,
candidates: FxIndexMap<Local, Vec<Local>>,
candidates_reverse: FxIndexMap<Local, Vec<Local>>,
write_info: WriteInfo,
// PERF: Do this for `MaybeLiveLocals` allocations too.
}
Expand All @@ -279,11 +277,11 @@ struct Candidates<'alloc> {
///
/// We will still report that we would like to merge `_1` and `_2` in an attempt to allow us to
/// remove that assignment.
c: &'alloc mut FxHashMap<Local, Vec<Local>>,
c: &'alloc mut FxIndexMap<Local, Vec<Local>>,
/// A reverse index of the `c` set; if the `c` set contains `a => Place { local: b, proj }`,
/// then this contains `b => a`.
// PERF: Possibly these should be `SmallVec`s?
reverse: &'alloc mut FxHashMap<Local, Vec<Local>>,
reverse: &'alloc mut FxIndexMap<Local, Vec<Local>>,
}

//////////////////////////////////////////////////////////
Expand All @@ -294,7 +292,7 @@ struct Candidates<'alloc> {
fn apply_merges<'tcx>(
body: &mut Body<'tcx>,
tcx: TyCtxt<'tcx>,
merges: &FxHashMap<Local, Local>,
merges: &FxIndexMap<Local, Local>,
merged_locals: &BitSet<Local>,
) {
let mut merger = Merger { tcx, merges, merged_locals };
Expand All @@ -303,7 +301,7 @@ fn apply_merges<'tcx>(

struct Merger<'a, 'tcx> {
tcx: TyCtxt<'tcx>,
merges: &'a FxHashMap<Local, Local>,
merges: &'a FxIndexMap<Local, Local>,
merged_locals: &'a BitSet<Local>,
}

Expand Down Expand Up @@ -386,7 +384,7 @@ impl<'alloc> Candidates<'alloc> {

/// `vec_filter_candidates` but for an `Entry`
fn entry_filter_candidates(
mut entry: OccupiedEntry<'_, Local, Vec<Local>>,
mut entry: IndexOccupiedEntry<'_, Local, Vec<Local>>,
p: Local,
f: impl FnMut(Local) -> CandidateFilter,
at: Location,
Expand All @@ -406,7 +404,7 @@ impl<'alloc> Candidates<'alloc> {
at: Location,
) {
// Cover the cases where `p` appears as a `src`
if let Entry::Occupied(entry) = self.c.entry(p) {
if let IndexEntry::Occupied(entry) = self.c.entry(p) {
Self::entry_filter_candidates(entry, p, &mut f, at);
}
// And the cases where `p` appears as a `dest`
Expand All @@ -419,7 +417,7 @@ impl<'alloc> Candidates<'alloc> {
if f(*src) == CandidateFilter::Keep {
return true;
}
let Entry::Occupied(entry) = self.c.entry(*src) else {
let IndexEntry::Occupied(entry) = self.c.entry(*src) else {
return false;
};
Self::entry_filter_candidates(
Expand Down Expand Up @@ -728,8 +726,8 @@ fn places_to_candidate_pair<'tcx>(
fn find_candidates<'alloc, 'tcx>(
body: &Body<'tcx>,
borrowed: &BitSet<Local>,
candidates: &'alloc mut FxHashMap<Local, Vec<Local>>,
candidates_reverse: &'alloc mut FxHashMap<Local, Vec<Local>>,
candidates: &'alloc mut FxIndexMap<Local, Vec<Local>>,
candidates_reverse: &'alloc mut FxIndexMap<Local, Vec<Local>>,
) -> Candidates<'alloc> {
candidates.clear();
candidates_reverse.clear();
Expand All @@ -751,7 +749,7 @@ fn find_candidates<'alloc, 'tcx>(

struct FindAssignments<'a, 'alloc, 'tcx> {
body: &'a Body<'tcx>,
candidates: &'alloc mut FxHashMap<Local, Vec<Local>>,
candidates: &'alloc mut FxIndexMap<Local, Vec<Local>>,
borrowed: &'a BitSet<Local>,
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,20 @@
let mut _3: u8;

bb0: {
- StorageLive(_2);
+ nop;
StorageLive(_3);
_3 = _1;
StorageLive(_2);
- StorageLive(_3);
- _3 = _1;
- _2 = dummy(move _3) -> [return: bb1, unwind unreachable];
+ _1 = dummy(move _3) -> [return: bb1, unwind unreachable];
+ nop;
+ nop;
+ _2 = dummy(move _1) -> [return: bb1, unwind unreachable];
}

bb1: {
StorageDead(_3);
- _1 = move _2;
- StorageDead(_2);
+ nop;
- StorageDead(_3);
+ nop;
_1 = move _2;
StorageDead(_2);
_0 = const ();
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,20 @@
let mut _3: u8;

bb0: {
- StorageLive(_2);
+ nop;
StorageLive(_3);
_3 = _1;
StorageLive(_2);
- StorageLive(_3);
- _3 = _1;
- _2 = dummy(move _3) -> [return: bb1, unwind continue];
+ _1 = dummy(move _3) -> [return: bb1, unwind continue];
+ nop;
+ nop;
+ _2 = dummy(move _1) -> [return: bb1, unwind continue];
}

bb1: {
StorageDead(_3);
- _1 = move _2;
- StorageDead(_2);
+ nop;
- StorageDead(_3);
+ nop;
_1 = move _2;
StorageDead(_2);
_0 = const ();
return;
}
Expand Down

0 comments on commit 6548f08

Please sign in to comment.