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

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`
2024-11-22 06:44:35 +00:00 · 2024-07-31 18:37:20 +00:00 · 2024-07-31 18:37:20 +00:00 · 28a58f2fa7
commit 28a58f2fa7
parent 99322d84c4 4067795d8c
11 changed files with 74 additions and 76 deletions
--- 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,6 +361,8 @@ 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 }))
@ -353,12 +370,16 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {

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

--- 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;
--- 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) { .. }
  
--- 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) { .. }
  
--- 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) { .. }
  
--- 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) { .. }
  
--- 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];
--- 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) {}
+  }
  
--- 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,13 +81,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: 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: {
@ -95,11 +94,9 @@
          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) {}
+  }
  
--- 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) {}
+  }
  
--- 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,13 +81,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: 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: {
@ -95,11 +94,9 @@
          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) {}
+  }