Skip to content

Commit 6db1e5e

Browse files
borsbors
committed
Auto merge of #111010 - scottmcm:mem-replace-simpler, r=WaffleLapkin
Make `mem::replace` simpler in codegen Since they'd mentioned more intrinsics for simplifying stuff recently, r? `@WaffleLapkin` This is a continuation of me looking at foundational stuff that ends up with more instructions than it really needs. Specifically I noticed this one because `Range::next` isn't MIR-inlining, and one of the largest parts of it is a `replace::<usize>` that's a good dozen instructions instead of the two it could be. So this means that `ptr::write` with a `Copy` type no longer generates worse IR than manually dereferencing (well, at least in LLVM -- MIR still has bonus pointer casts), and in doing so means that we're finally down to just the two essential `memcpy`s when emitting `mem::replace` for a large type, rather than the bonus-`alloca` and three `memcpy`s we emitted before this ([or the 6 we currently emit in 1.69 stable](https://rust.godbolt.org/z/67W8on6nP)). That said, LLVM does _usually_ manage to optimize the extra code away. But it's still nice for it not to have to do as much, thanks to (for example) not going through an `alloca` when `replace`ing a primitive like a `usize`. (This is a new intrinsic, but one that's immediately lowered to existing MIR constructs, so not anything that MIRI or the codegen backends or MIR semantics needs to do work to handle.)
2 parents b7d8c88 + 3456f77 commit 6db1e5e

17 files changed

+233
-59
lines changed

‎compiler/rustc_hir_analysis/src/check/intrinsic.rs

+1
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,7 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) {
381381
sym::unlikely => (0, vec![tcx.types.bool], tcx.types.bool),
382382

383383
sym::read_via_copy => (1, vec![tcx.mk_imm_ptr(param(0))], param(0)),
384+
sym::write_via_move => (1, vec![tcx.mk_mut_ptr(param(0)), param(0)], tcx.mk_unit()),
384385

385386
sym::discriminant_value => {
386387
let assoc_items = tcx.associated_item_def_ids(

‎compiler/rustc_mir_transform/src/lower_intrinsics.rs

+23
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,29 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics {
179179
}
180180
}
181181
}
182+
sym::write_via_move => {
183+
let target = target.unwrap();
184+
let Ok([ptr, val]) = <[_; 2]>::try_from(std::mem::take(args)) else {
185+
span_bug!(
186+
terminator.source_info.span,
187+
"Wrong number of arguments for write_via_move intrinsic",
188+
);
189+
};
190+
let derefed_place =
191+
if let Some(place) = ptr.place() && let Some(local) = place.as_local() {
192+
tcx.mk_place_deref(local.into())
193+
} else {
194+
span_bug!(terminator.source_info.span, "Only passing a local is supported");
195+
};
196+
block.statements.push(Statement {
197+
source_info: terminator.source_info,
198+
kind: StatementKind::Assign(Box::new((
199+
derefed_place,
200+
Rvalue::Use(val),
201+
))),
202+
});
203+
terminator.kind = TerminatorKind::Goto { target };
204+
}
182205
sym::discriminant_value => {
183206
if let (Some(target), Some(arg)) = (*target, args[0].place()) {
184207
let arg = tcx.mk_place_deref(arg);

‎compiler/rustc_span/src/symbol.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1636,6 +1636,7 @@ symbols! {
16361636
write_bytes,
16371637
write_macro,
16381638
write_str,
1639+
write_via_move,
16391640
writeln_macro,
16401641
x87_reg,
16411642
xer,

‎library/core/src/intrinsics.rs

+27-3
Original file line numberDiff line numberDiff line change
@@ -2257,12 +2257,23 @@ extern "rust-intrinsic" {
22572257
/// This is an implementation detail of [`crate::ptr::read`] and should
22582258
/// not be used anywhere else. See its comments for why this exists.
22592259
///
2260-
/// This intrinsic can *only* be called where the argument is a local without
2261-
/// projections (`read_via_copy(p)`, not `read_via_copy(*p)`) so that it
2260+
/// This intrinsic can *only* be called where the pointer is a local without
2261+
/// projections (`read_via_copy(ptr)`, not `read_via_copy(*ptr)`) so that it
22622262
/// trivially obeys runtime-MIR rules about derefs in operands.
22632263
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
22642264
#[rustc_nounwind]
2265-
pub fn read_via_copy<T>(p: *const T) -> T;
2265+
pub fn read_via_copy<T>(ptr: *const T) -> T;
2266+
2267+
/// This is an implementation detail of [`crate::ptr::write`] and should
2268+
/// not be used anywhere else. See its comments for why this exists.
2269+
///
2270+
/// This intrinsic can *only* be called where the pointer is a local without
2271+
/// projections (`write_via_move(ptr, x)`, not `write_via_move(*ptr, x)`) so
2272+
/// that it trivially obeys runtime-MIR rules about derefs in operands.
2273+
#[cfg(not(bootstrap))]
2274+
#[rustc_const_unstable(feature = "const_ptr_write", issue = "86302")]
2275+
#[rustc_nounwind]
2276+
pub fn write_via_move<T>(ptr: *mut T, value: T);
22662277

22672278
/// Returns the value of the discriminant for the variant in 'v';
22682279
/// if `T` has no discriminant, returns `0`.
@@ -2828,3 +2839,16 @@ pub const unsafe fn transmute_unchecked<Src, Dst>(src: Src) -> Dst {
28282839
// SAFETY: It's a transmute -- the caller promised it's fine.
28292840
unsafe { transmute_copy(&ManuallyDrop::new(src)) }
28302841
}
2842+
2843+
/// Polyfill for bootstrap
2844+
#[cfg(bootstrap)]
2845+
pub const unsafe fn write_via_move<T>(ptr: *mut T, value: T) {
2846+
use crate::mem::*;
2847+
// SAFETY: the caller must guarantee that `dst` is valid for writes.
2848+
// `dst` cannot overlap `src` because the caller has mutable access
2849+
// to `dst` while `src` is owned by this function.
2850+
unsafe {
2851+
copy_nonoverlapping::<T>(&value, ptr, 1);
2852+
forget(value);
2853+
}
2854+
}

‎library/core/src/ptr/mod.rs

+8-9
Original file line numberDiff line numberDiff line change
@@ -1349,13 +1349,13 @@ pub const unsafe fn read_unaligned<T>(src: *const T) -> T {
13491349
#[rustc_const_unstable(feature = "const_ptr_write", issue = "86302")]
13501350
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
13511351
pub const unsafe fn write<T>(dst: *mut T, src: T) {
1352-
// We are calling the intrinsics directly to avoid function calls in the generated code
1353-
// as `intrinsics::copy_nonoverlapping` is a wrapper function.
1354-
extern "rust-intrinsic" {
1355-
#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")]
1356-
#[rustc_nounwind]
1357-
fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize);
1358-
}
1352+
// Semantically, it would be fine for this to be implemented as a
1353+
// `copy_nonoverlapping` and appropriate drop suppression of `src`.
1354+
1355+
// However, implementing via that currently produces more MIR than is ideal.
1356+
// Using an intrinsic keeps it down to just the simple `*dst = move src` in
1357+
// MIR (11 statements shorter, at the time of writing), and also allows
1358+
// `src` to stay an SSA value in codegen_ssa, rather than a memory one.
13591359

13601360
// SAFETY: the caller must guarantee that `dst` is valid for writes.
13611361
// `dst` cannot overlap `src` because the caller has mutable access
@@ -1365,8 +1365,7 @@ pub const unsafe fn write<T>(dst: *mut T, src: T) {
13651365
"ptr::write requires that the pointer argument is aligned and non-null",
13661366
[T](dst: *mut T) => is_aligned_and_not_null(dst)
13671367
);
1368-
copy_nonoverlapping(&src as *const T, dst, 1);
1369-
intrinsics::forget(src);
1368+
intrinsics::write_via_move(dst, src)
13701369
}
13711370
}
13721371

‎src/tools/miri/tests/fail/dangling_pointers/null_pointer_write_zst.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,5 @@ fn main() {
77
// Also not assigning directly as that's array initialization, not assignment.
88
let zst_val = [1u8; 0];
99
unsafe { std::ptr::null_mut::<[u8; 0]>().write(zst_val) };
10-
//~^ERROR: memory access failed: null pointer is a dangling pointer
10+
//~^ERROR: dereferencing pointer failed: null pointer is a dangling pointer
1111
}

‎src/tools/miri/tests/fail/dangling_pointers/null_pointer_write_zst.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
error: Undefined Behavior: memory access failed: null pointer is a dangling pointer (it has no provenance)
1+
error: Undefined Behavior: dereferencing pointer failed: null pointer is a dangling pointer (it has no provenance)
22
--> $DIR/null_pointer_write_zst.rs:LL:CC
33
|
44
LL | unsafe { std::ptr::null_mut::<[u8; 0]>().write(zst_val) };
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: null pointer is a dangling pointer (it has no provenance)
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ dereferencing pointer failed: null pointer is a dangling pointer (it has no provenance)
66
|
77
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
88
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

‎tests/codegen/mem-replace-big-type.rs

+6-8
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@
1111
#[repr(C, align(8))]
1212
pub struct Big([u64; 7]);
1313
pub fn replace_big(dst: &mut Big, src: Big) -> Big {
14-
// Before the `read_via_copy` intrinsic, this emitted six `memcpy`s.
14+
// Back in 1.68, this emitted six `memcpy`s.
15+
// `read_via_copy` in 1.69 got that down to three.
16+
// `write_via_move` has it down to just the two essential ones.
1517
std::mem::replace(dst, src)
1618
}
1719

@@ -20,17 +22,13 @@ pub fn replace_big(dst: &mut Big, src: Big) -> Big {
2022

2123
// CHECK-NOT: call void @llvm.memcpy
2224

23-
// For a large type, we expect exactly three `memcpy`s
25+
// For a large type, we expect exactly two `memcpy`s
2426
// CHECK-LABEL: define internal void @{{.+}}mem{{.+}}replace{{.+}}sret(%Big)
2527
// CHECK-NOT: alloca
26-
// CHECK: alloca %Big
27-
// CHECK-NOT: alloca
28-
// CHECK-NOT: call void @llvm.memcpy
29-
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false)
3028
// CHECK-NOT: call void @llvm.memcpy
31-
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false)
29+
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %0, {{i8\*|ptr}} align 8 %dest, i{{.*}} 56, i1 false)
3230
// CHECK-NOT: call void @llvm.memcpy
33-
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %{{.*}}, {{i8\*|ptr}} align 8 %{{.*}}, i{{.*}} 56, i1 false)
31+
// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 8 %dest, {{i8\*|ptr}} align 8 %src, i{{.*}} 56, i1 false)
3432
// CHECK-NOT: call void @llvm.memcpy
3533

3634
// CHECK-NOT: call void @llvm.memcpy

‎tests/codegen/mem-replace-direct-memcpy.rs

-33
This file was deleted.

‎tests/codegen/mem-replace-simple-type.rs

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// compile-flags: -O -C no-prepopulate-passes
2+
// min-llvm-version: 15.0 (for opaque pointers)
3+
// only-x86_64 (to not worry about usize differing)
4+
// ignore-debug (the debug assertions get in the way)
5+
6+
#![crate_type = "lib"]
7+
8+
#[no_mangle]
9+
// CHECK-LABEL: @replace_usize(
10+
pub fn replace_usize(r: &mut usize, v: usize) -> usize {
11+
// CHECK-NOT: alloca
12+
// CHECK: %[[R:.+]] = load i64, ptr %r
13+
// CHECK: store i64 %v, ptr %r
14+
// CHECK: ret i64 %[[R]]
15+
std::mem::replace(r, v)
16+
}
17+
18+
#[no_mangle]
19+
// CHECK-LABEL: @replace_ref_str(
20+
pub fn replace_ref_str<'a>(r: &mut &'a str, v: &'a str) -> &'a str {
21+
// CHECK-NOT: alloca
22+
// CHECK: %[[A:.+]] = load ptr
23+
// CHECK: %[[B:.+]] = load i64
24+
// CHECK-NOT: store
25+
// CHECK-NOT: load
26+
// CHECK: store ptr
27+
// CHECK: store i64
28+
// CHECK-NOT: load
29+
// CHECK-NOT: store
30+
// CHECK: %[[P1:.+]] = insertvalue { ptr, i64 } poison, ptr %[[A]], 0
31+
// CHECK: %[[P2:.+]] = insertvalue { ptr, i64 } %[[P1]], i64 %[[B]], 1
32+
// CHECK: ret { ptr, i64 } %[[P2]]
33+
std::mem::replace(r, v)
34+
}

‎tests/mir-opt/lower_intrinsics.option_payload.LowerIntrinsics.diff

+2-2
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
_4 = &raw const (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+2:55: +2:56
2525
- _3 = option_payload_ptr::<usize>(move _4) -> [return: bb1, unwind unreachable]; // scope 1 at $DIR/lower_intrinsics.rs:+2:18: +2:57
2626
- // mir::Constant
27-
- // + span: $DIR/lower_intrinsics.rs:132:18: 132:54
27+
- // + span: $DIR/lower_intrinsics.rs:137:18: 137:54
2828
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Option<usize>) -> *const usize {option_payload_ptr::<usize>}, val: Value(<ZST>) }
2929
+ _3 = &raw const (((*_4) as Some).0: usize); // scope 1 at $DIR/lower_intrinsics.rs:+2:18: +2:57
3030
+ goto -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+2:18: +2:57
@@ -37,7 +37,7 @@
3737
_6 = &raw const (*_2); // scope 2 at $DIR/lower_intrinsics.rs:+3:55: +3:56
3838
- _5 = option_payload_ptr::<String>(move _6) -> [return: bb2, unwind unreachable]; // scope 2 at $DIR/lower_intrinsics.rs:+3:18: +3:57
3939
- // mir::Constant
40-
- // + span: $DIR/lower_intrinsics.rs:133:18: 133:54
40+
- // + span: $DIR/lower_intrinsics.rs:138:18: 138:54
4141
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Option<String>) -> *const String {option_payload_ptr::<String>}, val: Value(<ZST>) }
4242
+ _5 = &raw const (((*_6) as Some).0: std::string::String); // scope 2 at $DIR/lower_intrinsics.rs:+3:18: +3:57
4343
+ goto -> bb2; // scope 2 at $DIR/lower_intrinsics.rs:+3:18: +3:57

‎tests/mir-opt/lower_intrinsics.ptr_offset.LowerIntrinsics.diff

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
_4 = _2; // scope 0 at $DIR/lower_intrinsics.rs:+1:33: +1:34
1616
- _0 = offset::<*const i32, isize>(move _3, move _4) -> [return: bb1, unwind unreachable]; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
1717
- // mir::Constant
18-
- // + span: $DIR/lower_intrinsics.rs:139:5: 139:29
18+
- // + span: $DIR/lower_intrinsics.rs:144:5: 144:29
1919
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const i32, isize) -> *const i32 {offset::<*const i32, isize>}, val: Value(<ZST>) }
2020
+ _0 = Offset(move _3, move _4); // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
2121
+ goto -> bb1; // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35

‎tests/mir-opt/lower_intrinsics.rs

+5
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,11 @@ pub fn read_via_copy_uninhabited(r: &Never) -> Never {
124124
unsafe { core::intrinsics::read_via_copy(r) }
125125
}
126126

127+
// EMIT_MIR lower_intrinsics.write_via_move_string.LowerIntrinsics.diff
128+
pub fn write_via_move_string(r: &mut String, v: String) {
129+
unsafe { core::intrinsics::write_via_move(r, v) }
130+
}
131+
127132
pub enum Never {}
128133

129134
// EMIT_MIR lower_intrinsics.option_payload.LowerIntrinsics.diff

‎tests/mir-opt/lower_intrinsics.write_via_move_string.LowerIntrinsics.diff

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
- // MIR for `write_via_move_string` before LowerIntrinsics
2+
+ // MIR for `write_via_move_string` after LowerIntrinsics
3+
4+
fn write_via_move_string(_1: &mut String, _2: String) -> () {
5+
debug r => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:30: +0:31
6+
debug v => _2; // in scope 0 at $DIR/lower_intrinsics.rs:+0:46: +0:47
7+
let mut _0: (); // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:57: +0:57
8+
let mut _3: *mut std::string::String; // in scope 0 at $DIR/lower_intrinsics.rs:+1:47: +1:48
9+
let mut _4: std::string::String; // in scope 0 at $DIR/lower_intrinsics.rs:+1:50: +1:51
10+
scope 1 {
11+
}
12+
13+
bb0: {
14+
StorageLive(_3); // scope 1 at $DIR/lower_intrinsics.rs:+1:47: +1:48
15+
_3 = &raw mut (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+1:47: +1:48
16+
StorageLive(_4); // scope 1 at $DIR/lower_intrinsics.rs:+1:50: +1:51
17+
_4 = move _2; // scope 1 at $DIR/lower_intrinsics.rs:+1:50: +1:51
18+
- _0 = write_via_move::<String>(move _3, move _4) -> [return: bb1, unwind unreachable]; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:52
19+
- // mir::Constant
20+
- // + span: $DIR/lower_intrinsics.rs:129:14: 129:46
21+
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*mut String, String) {write_via_move::<String>}, val: Value(<ZST>) }
22+
+ (*_3) = move _4; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:52
23+
+ goto -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:52
24+
}
25+
26+
bb1: {
27+
StorageDead(_4); // scope 1 at $DIR/lower_intrinsics.rs:+1:51: +1:52
28+
StorageDead(_3); // scope 1 at $DIR/lower_intrinsics.rs:+1:51: +1:52
29+
goto -> bb2; // scope 0 at $DIR/lower_intrinsics.rs:+2:1: +2:2
30+
}
31+
32+
bb2: {
33+
return; // scope 0 at $DIR/lower_intrinsics.rs:+2:2: +2:2
34+
}
35+
}
36+

‎tests/mir-opt/pre-codegen/mem_replace.manual_replace.PreCodegen.after.mir

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// MIR for `manual_replace` after PreCodegen
2+
3+
fn manual_replace(_1: &mut u32, _2: u32) -> u32 {
4+
debug r => _1; // in scope 0 at $DIR/mem_replace.rs:+0:23: +0:24
5+
debug v => _2; // in scope 0 at $DIR/mem_replace.rs:+0:36: +0:37
6+
let mut _0: u32; // return place in scope 0 at $DIR/mem_replace.rs:+1:9: +1:13
7+
scope 1 {
8+
debug temp => _0; // in scope 1 at $DIR/mem_replace.rs:+1:9: +1:13
9+
}
10+
11+
bb0: {
12+
_0 = (*_1); // scope 0 at $DIR/mem_replace.rs:+1:16: +1:18
13+
(*_1) = _2; // scope 1 at $DIR/mem_replace.rs:+2:5: +2:11
14+
return; // scope 0 at $DIR/mem_replace.rs:+4:2: +4:2
15+
}
16+
}

0 commit comments

Comments
 (0)