Auto merge of #126991 - cjgillot:gvn-prof, r=oli-obk

bors · bors · commit 28a58f2fa7f0 · 2024-07-31T18:37:20.000Z
Accelerate GVN a little

This PR addresses a few inefficiencies I've seen in callgrind profiles.

Commits are independent.

Only the first commit introduces a change in behaviour: we stop substituting some constant pointers. But we keep propagating their contents that have no provenance, so we don't lose much.

r? `@saethlin`
diff --git a/compiler/rustc_mir_transform/src/gvn.rs b/compiler/rustc_mir_transform/src/gvn.rs
@@ -128,7 +128,7 @@ fn propagate_ssa<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
     // Clone dominators as we need them while mutating the body.
     let dominators = body.basic_blocks.dominators().clone();
 
-    let mut state = VnState::new(tcx, param_env, &ssa, &dominators, &body.local_decls);
+    let mut state = VnState::new(tcx, body, param_env, &ssa, &dominators, &body.local_decls);
     ssa.for_each_assignment_mut(
         body.basic_blocks.as_mut_preserves_cfg(),
         |local, value, location| {
@@ -204,6 +204,7 @@ enum Value<'tcx> {
         value: Const<'tcx>,
         /// Some constants do not have a deterministic value. To avoid merging two instances of the
         /// same `Const`, we assign them an additional integer index.
+        // `disambiguator` is 0 iff the constant is deterministic.
         disambiguator: usize,
     },
     /// An aggregate value, either tuple/closure/struct/enum.
@@ -266,21 +267,29 @@ struct VnState<'body, 'tcx> {
 impl<'body, 'tcx> VnState<'body, 'tcx> {
     fn new(
         tcx: TyCtxt<'tcx>,
+        body: &Body<'tcx>,
         param_env: ty::ParamEnv<'tcx>,
         ssa: &'body SsaLocals,
         dominators: &'body Dominators<BasicBlock>,
         local_decls: &'body LocalDecls<'tcx>,
     ) -> Self {
+        // Compute a rough estimate of the number of values in the body from the number of
+        // statements. This is meant to reduce the number of allocations, but it's all right if
+        // we miss the exact amount. We estimate based on 2 values per statement (one in LHS and
+        // one in RHS) and 4 values per terminator (for call operands).
+        let num_values =
+            2 * body.basic_blocks.iter().map(|bbdata| bbdata.statements.len()).sum::<usize>()
+                + 4 * body.basic_blocks.len();
         VnState {
             tcx,
             ecx: InterpCx::new(tcx, DUMMY_SP, param_env, DummyMachine),
             param_env,
             local_decls,
             locals: IndexVec::from_elem(None, local_decls),
-            rev_locals: IndexVec::default(),
-            values: FxIndexSet::default(),
-            evaluated: IndexVec::new(),
-            next_opaque: Some(0),
+            rev_locals: IndexVec::with_capacity(num_values),
+            values: FxIndexSet::with_capacity_and_hasher(num_values, Default::default()),
+            evaluated: IndexVec::with_capacity(num_values),
+            next_opaque: Some(1),
             feature_unsized_locals: tcx.features().unsized_locals,
             ssa,
             dominators,
@@ -293,9 +302,15 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
         let (index, new) = self.values.insert_full(value);
         let index = VnIndex::from_usize(index);
         if new {
+            // Grow `evaluated` and `rev_locals` here to amortize the allocations.
             let evaluated = self.eval_to_const(index);
             let _index = self.evaluated.push(evaluated);
             debug_assert_eq!(index, _index);
+            // No need to push to `rev_locals` if we finished listing assignments.
+            if self.next_opaque.is_some() {
+                let _index = self.rev_locals.push(SmallVec::new());
+                debug_assert_eq!(index, _index);
+            }
         }
         index
     }
@@ -332,7 +347,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
         let is_sized = !self.feature_unsized_locals
             || self.local_decls[local].ty.is_sized(self.tcx, self.param_env);
         if is_sized {
-            self.rev_locals.ensure_contains_elem(value, SmallVec::new).push(local);
+            self.rev_locals[value].push(local);
         }
     }
 
@@ -346,19 +361,25 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
             let next_opaque = self.next_opaque.as_mut()?;
             let disambiguator = *next_opaque;
             *next_opaque += 1;
+            // `disambiguator: 0` means deterministic.
+            debug_assert_ne!(disambiguator, 0);
             disambiguator
         };
         Some(self.insert(Value::Constant { value, disambiguator }))
     }
 
     fn insert_bool(&mut self, flag: bool) -> VnIndex {
         // Booleans are deterministic.
-        self.insert(Value::Constant { value: Const::from_bool(self.tcx, flag), disambiguator: 0 })
+        let value = Const::from_bool(self.tcx, flag);
+        debug_assert!(value.is_deterministic());
+        self.insert(Value::Constant { value, disambiguator: 0 })
     }
 
     fn insert_scalar(&mut self, scalar: Scalar, ty: Ty<'tcx>) -> VnIndex {
-        self.insert_constant(Const::from_scalar(self.tcx, scalar, ty))
-            .expect("scalars are deterministic")
+        // Scalars are deterministic.
+        let value = Const::from_scalar(self.tcx, scalar, ty);
+        debug_assert!(value.is_deterministic());
+        self.insert(Value::Constant { value, disambiguator: 0 })
     }
 
     fn insert_tuple(&mut self, values: Vec<VnIndex>) -> VnIndex {
@@ -671,7 +692,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
     fn simplify_place_projection(&mut self, place: &mut Place<'tcx>, location: Location) {
         // If the projection is indirect, we treat the local as a value, so can replace it with
         // another local.
-        if place.is_indirect()
+        if place.is_indirect_first_projection()
             && let Some(base) = self.locals[place.local]
             && let Some(new_local) = self.try_as_local(base, location)
             && place.local != new_local
@@ -773,10 +794,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
         location: Location,
     ) -> Option<VnIndex> {
         match *operand {
-            Operand::Constant(ref mut constant) => {
-                let const_ = constant.const_.normalize(self.tcx, self.param_env);
-                self.insert_constant(const_)
-            }
+            Operand::Constant(ref constant) => self.insert_constant(constant.const_),
             Operand::Copy(ref mut place) | Operand::Move(ref mut place) => {
                 let value = self.simplify_place_value(place, location)?;
                 if let Some(const_) = self.try_as_constant(value) {
@@ -1371,8 +1389,13 @@ fn op_to_prop_const<'tcx>(
     // If this constant has scalar ABI, return it as a `ConstValue::Scalar`.
     if let Abi::Scalar(abi::Scalar::Initialized { .. }) = op.layout.abi
         && let Ok(scalar) = ecx.read_scalar(op)
-        && scalar.try_to_scalar_int().is_ok()
     {
+        if !scalar.try_to_scalar_int().is_ok() {
+            // Check that we do not leak a pointer.
+            // Those pointers may lose part of their identity in codegen.
+            // FIXME: remove this hack once https://github.com/rust-lang/rust/issues/79738 is fixed.
+            return None;
+        }
         return Some(ConstValue::Scalar(scalar));
     }
 
@@ -1436,12 +1459,11 @@ impl<'tcx> VnState<'_, 'tcx> {
 
     /// If `index` is a `Value::Constant`, return the `Constant` to be put in the MIR.
     fn try_as_constant(&mut self, index: VnIndex) -> Option<ConstOperand<'tcx>> {
-        // This was already constant in MIR, do not change it.
-        if let Value::Constant { value, disambiguator: _ } = *self.get(index)
-            // If the constant is not deterministic, adding an additional mention of it in MIR will
-            // not give the same value as the former mention.
-            && value.is_deterministic()
-        {
+        // This was already constant in MIR, do not change it. If the constant is not
+        // deterministic, adding an additional mention of it in MIR will not give the same value as
+        // the former mention.
+        if let Value::Constant { value, disambiguator: 0 } = *self.get(index) {
+            debug_assert!(value.is_deterministic());
             return Some(ConstOperand { span: DUMMY_SP, user_ty: None, const_: value });
         }
 
diff --git a/tests/incremental/hashes/for_loops.rs b/tests/incremental/hashes/for_loops.rs
@@ -103,9 +103,9 @@ pub fn change_iterable() {
 }
 
 #[cfg(not(any(cfail1,cfail4)))]
-#[rustc_clean(cfg="cfail2", except="opt_hir_owner_nodes, promoted_mir, optimized_mir")]
+#[rustc_clean(cfg="cfail2", except="opt_hir_owner_nodes, promoted_mir")]
 #[rustc_clean(cfg="cfail3")]
-#[rustc_clean(cfg="cfail5", except="opt_hir_owner_nodes, promoted_mir, optimized_mir")]
+#[rustc_clean(cfg="cfail5", except="opt_hir_owner_nodes, promoted_mir")]
 #[rustc_clean(cfg="cfail6")]
 pub fn change_iterable() {
     let mut _x = 0;
diff --git a/tests/mir-opt/const_prop/slice_len.main.GVN.32bit.panic-abort.diff b/tests/mir-opt/const_prop/slice_len.main.GVN.32bit.panic-abort.diff
@@ -22,12 +22,11 @@
           StorageLive(_3);
           StorageLive(_4);
           _9 = const main::promoted[0];
--         _4 = _9;
+          _4 = _9;
 -         _3 = _4;
 -         _2 = move _3 as &[u32] (PointerCoercion(Unsize));
-+         _4 = const {ALLOC0<imm>: &[u32; 3]};
-+         _3 = const {ALLOC0<imm>: &[u32; 3]};
-+         _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
++         _3 = _9;
++         _2 = _9 as &[u32] (PointerCoercion(Unsize));
           StorageDead(_3);
           StorageLive(_6);
           _6 = const 1_usize;
@@ -50,6 +49,4 @@
           return;
       }
   }
-+ 
-+ ALLOC0 (size: 12, align: 4) { .. }
   
diff --git a/tests/mir-opt/const_prop/slice_len.main.GVN.32bit.panic-unwind.diff b/tests/mir-opt/const_prop/slice_len.main.GVN.32bit.panic-unwind.diff
@@ -22,12 +22,11 @@
           StorageLive(_3);
           StorageLive(_4);
           _9 = const main::promoted[0];
--         _4 = _9;
+          _4 = _9;
 -         _3 = _4;
 -         _2 = move _3 as &[u32] (PointerCoercion(Unsize));
-+         _4 = const {ALLOC0<imm>: &[u32; 3]};
-+         _3 = const {ALLOC0<imm>: &[u32; 3]};
-+         _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
++         _3 = _9;
++         _2 = _9 as &[u32] (PointerCoercion(Unsize));
           StorageDead(_3);
           StorageLive(_6);
           _6 = const 1_usize;
@@ -50,6 +49,4 @@
           return;
       }
   }
-+ 
-+ ALLOC0 (size: 12, align: 4) { .. }
   
diff --git a/tests/mir-opt/const_prop/slice_len.main.GVN.64bit.panic-abort.diff b/tests/mir-opt/const_prop/slice_len.main.GVN.64bit.panic-abort.diff
@@ -22,12 +22,11 @@
           StorageLive(_3);
           StorageLive(_4);
           _9 = const main::promoted[0];
--         _4 = _9;
+          _4 = _9;
 -         _3 = _4;
 -         _2 = move _3 as &[u32] (PointerCoercion(Unsize));
-+         _4 = const {ALLOC0<imm>: &[u32; 3]};
-+         _3 = const {ALLOC0<imm>: &[u32; 3]};
-+         _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
++         _3 = _9;
++         _2 = _9 as &[u32] (PointerCoercion(Unsize));
           StorageDead(_3);
           StorageLive(_6);
           _6 = const 1_usize;
@@ -50,6 +49,4 @@
           return;
       }
   }
-+ 
-+ ALLOC0 (size: 12, align: 4) { .. }
   
diff --git a/tests/mir-opt/const_prop/slice_len.main.GVN.64bit.panic-unwind.diff b/tests/mir-opt/const_prop/slice_len.main.GVN.64bit.panic-unwind.diff
@@ -22,12 +22,11 @@
           StorageLive(_3);
           StorageLive(_4);
           _9 = const main::promoted[0];
--         _4 = _9;
+          _4 = _9;
 -         _3 = _4;
 -         _2 = move _3 as &[u32] (PointerCoercion(Unsize));
-+         _4 = const {ALLOC0<imm>: &[u32; 3]};
-+         _3 = const {ALLOC0<imm>: &[u32; 3]};
-+         _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
++         _3 = _9;
++         _2 = _9 as &[u32] (PointerCoercion(Unsize));
           StorageDead(_3);
           StorageLive(_6);
           _6 = const 1_usize;
@@ -50,6 +49,4 @@
           return;
       }
   }
-+ 
-+ ALLOC0 (size: 12, align: 4) { .. }
   
diff --git a/tests/mir-opt/const_prop/slice_len.rs b/tests/mir-opt/const_prop/slice_len.rs
@@ -7,7 +7,7 @@
 fn main() {
     // CHECK-LABEL: fn main(
     // CHECK: debug a => [[a:_.*]];
-    // CHECK: [[slice:_.*]] = const {{.*}} as &[u32] (PointerCoercion(Unsize));
+    // CHECK: [[slice:_.*]] = {{.*}} as &[u32] (PointerCoercion(Unsize));
     // CHECK: assert(const true,
     // CHECK: [[a]] = const 2_u32;
     let a = (&[1u32, 2, 3] as &[u32])[1];
diff --git a/tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.32bit.panic-abort.diff b/tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.32bit.panic-abort.diff
@@ -73,13 +73,12 @@
           StorageLive(_6);
           StorageLive(_7);
           _9 = const main::promoted[0];
--         _7 = _9;
-+         _7 = const {ALLOC1<imm>: &std::alloc::Global};
+          _7 = _9;
           StorageLive(_8);
 -         _8 = _1;
 -         _6 = std::alloc::Global::alloc_impl(move _7, move _8, const false) -> [return: bb4, unwind unreachable];
 +         _8 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }};
-+         _6 = std::alloc::Global::alloc_impl(const {ALLOC1<imm>: &std::alloc::Global}, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb4, unwind unreachable];
++         _6 = std::alloc::Global::alloc_impl(_9, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb4, unwind unreachable];
       }
   
       bb4: {
@@ -119,11 +118,9 @@
 +         nop;
           return;
       }
-  }
++ }
 + 
 + ALLOC0 (size: 8, align: 4) {
 +     00 00 00 00 __ __ __ __                         │ ....░░░░
-+ }
-+ 
-+ ALLOC1 (size: 0, align: 1) {}
+  }
   
diff --git a/tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.32bit.panic-unwind.diff b/tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.32bit.panic-unwind.diff
@@ -81,25 +81,22 @@
           StorageLive(_6);
           StorageLive(_7);
           _9 = const main::promoted[0];
--         _7 = _9;
-+         _7 = const {ALLOC1<imm>: &std::alloc::Global};
+          _7 = _9;
           StorageLive(_8);
 -         _8 = _1;
 -         _6 = std::alloc::Global::alloc_impl(move _7, move _8, const false) -> [return: bb5, unwind continue];
 +         _8 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }};
-+         _6 = std::alloc::Global::alloc_impl(const {ALLOC1<imm>: &std::alloc::Global}, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb5, unwind continue];
++         _6 = std::alloc::Global::alloc_impl(_9, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb5, unwind continue];
       }
   
       bb5: {
           StorageDead(_8);
           StorageDead(_7);
           _5 = Result::<NonNull<[u8]>, std::alloc::AllocError>::unwrap(move _6) -> [return: bb1, unwind continue];
       }
-  }
++ }
 + 
 + ALLOC0 (size: 8, align: 4) {
 +     00 00 00 00 __ __ __ __                         │ ....░░░░
-+ }
-+ 
-+ ALLOC1 (size: 0, align: 1) {}
+  }
   
diff --git a/tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.64bit.panic-abort.diff b/tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.64bit.panic-abort.diff
@@ -73,13 +73,12 @@
           StorageLive(_6);
           StorageLive(_7);
           _9 = const main::promoted[0];
--         _7 = _9;
-+         _7 = const {ALLOC1<imm>: &std::alloc::Global};
+          _7 = _9;
           StorageLive(_8);
 -         _8 = _1;
 -         _6 = std::alloc::Global::alloc_impl(move _7, move _8, const false) -> [return: bb4, unwind unreachable];
 +         _8 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }};
-+         _6 = std::alloc::Global::alloc_impl(const {ALLOC1<imm>: &std::alloc::Global}, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb4, unwind unreachable];
++         _6 = std::alloc::Global::alloc_impl(_9, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb4, unwind unreachable];
       }
   
       bb4: {
@@ -119,11 +118,9 @@
 +         nop;
           return;
       }
-  }
++ }
 + 
 + ALLOC0 (size: 16, align: 8) {
 +     00 00 00 00 00 00 00 00 __ __ __ __ __ __ __ __ │ ........░░░░░░░░
-+ }
-+ 
-+ ALLOC1 (size: 0, align: 1) {}
+  }
   
diff --git a/tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.64bit.panic-unwind.diff b/tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.64bit.panic-unwind.diff
@@ -81,25 +81,22 @@
           StorageLive(_6);
           StorageLive(_7);
           _9 = const main::promoted[0];
--         _7 = _9;
-+         _7 = const {ALLOC1<imm>: &std::alloc::Global};
+          _7 = _9;
           StorageLive(_8);
 -         _8 = _1;
 -         _6 = std::alloc::Global::alloc_impl(move _7, move _8, const false) -> [return: bb5, unwind continue];
 +         _8 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }};
-+         _6 = std::alloc::Global::alloc_impl(const {ALLOC1<imm>: &std::alloc::Global}, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb5, unwind continue];
++         _6 = std::alloc::Global::alloc_impl(_9, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb5, unwind continue];
       }
   
       bb5: {
           StorageDead(_8);
           StorageDead(_7);
           _5 = Result::<NonNull<[u8]>, std::alloc::AllocError>::unwrap(move _6) -> [return: bb1, unwind continue];
       }
-  }
++ }
 + 
 + ALLOC0 (size: 16, align: 8) {
 +     00 00 00 00 00 00 00 00 __ __ __ __ __ __ __ __ │ ........░░░░░░░░
-+ }
-+ 
-+ ALLOC1 (size: 0, align: 1) {}
+  }
   
