mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rollup merge of #119591 - Enselic:DestinationPropagation-stable, r=cjgillot

Browse Source 
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):
* 090d5eac72/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff

With this diff (after fix):
* f603babd63/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](https://github.com/rust-lang/rust/pull/119252#discussion_r1436101791) comment, but I wanted to make a separate PR for this fix for a simpler development and review process.

Part of https://github.com/rust-lang/rust/issues/84447 which is E-help-wanted.

r? `@cjgillot` who is reviewer for the highly related PR https://github.com/rust-lang/rust/pull/119252.
This commit is contained in:
Matthias Krüger 2024-01-06 16:07:47 +01:00 committed by GitHub
commit 909f2b63a3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 33 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
compiler/rustc_data_structures/src/fx.rs
View File

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

30
compiler/rustc_mir_transform/src/dest_prop.rs
View File

@ -131,10 +131,8 @@
//! [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::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;
@ -211,7 +209,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) {
@ -250,8 +248,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.
}
@ -272,11 +270,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>>,
}
//////////////////////////////////////////////////////////
@ -287,7 +285,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 };
@ -296,7 +294,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>,
}
@ -379,7 +377,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,
@ -399,7 +397,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`
@ -412,7 +410,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(
@ -721,8 +719,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();
@ -744,7 +742,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>,
}

18
tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-abort.diff
View File

@ -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;
}

18
tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff
View File

@ -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;
}