about summary refs log tree commit diff
path: root/compiler/rustc_mir_transform/src
diff options
context:
space:
mode:
authorMatthias Krüger <matthias.krueger@famsik.de>2024-01-06 16:07:47 +0100
committerGitHub <noreply@github.com>2024-01-06 16:07:47 +0100
commit909f2b63a3c1c1d4a9fa1bf075faa21ff77297ec (patch)
tree3503388e1110a1a2a14a7bcfc36c871c96502972 /compiler/rustc_mir_transform/src
parent94bfc28bcd5ca68b77d1ee13e52c18b303589647 (diff)
parent95eb5bcb67b97e7a1f3f6e6e68b3a8a0737ca24e (diff)
downloadrust-909f2b63a3c1c1d4a9fa1bf075faa21ff77297ec.tar.gz
rust-909f2b63a3c1c1d4a9fa1bf075faa21ff77297ec.zip
Rollup merge of #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](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.
Diffstat (limited to 'compiler/rustc_mir_transform/src')
-rw-r--r--compiler/rustc_mir_transform/src/dest_prop.rs30
1 files changed, 14 insertions, 16 deletions
diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs
index cd80f423466..49b0edc0db8 100644
--- a/compiler/rustc_mir_transform/src/dest_prop.rs
+++ b/compiler/rustc_mir_transform/src/dest_prop.rs
@@ -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>,
 }