Skip to content

Commit e7c6ad8

Browse files
scottmcmscottmcm
committed
Improved implementation and comments after code review feedback
1 parent 87696fd commit e7c6ad8

File tree

6 files changed

+124
-77
lines changed

6 files changed

+124
-77
lines changed

‎compiler/rustc_mir_transform/src/lower_intrinsics.rs

+18-13
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics {
150150
}
151151
}
152152
sym::read_via_copy => {
153-
let Ok([arg]) = <[_; 1]>::try_from(std::mem::take(args)) else {
153+
let [arg] = args.as_slice() else {
154154
span_bug!(terminator.source_info.span, "Wrong number of arguments");
155155
};
156156
let derefed_place =
@@ -159,18 +159,23 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics {
159159
} else {
160160
span_bug!(terminator.source_info.span, "Only passing a local is supported");
161161
};
162-
block.statements.push(Statement {
163-
source_info: terminator.source_info,
164-
kind: StatementKind::Assign(Box::new((
165-
*destination,
166-
Rvalue::Use(Operand::Copy(derefed_place)),
167-
))),
168-
});
169-
if let Some(target) = *target {
170-
terminator.kind = TerminatorKind::Goto { target };
171-
} else {
172-
// Reading something uninhabited means this is unreachable.
173-
terminator.kind = TerminatorKind::Unreachable;
162+
terminator.kind = match *target {
163+
None => {
164+
// No target means this read something uninhabited,
165+
// so it must be unreachable, and we don't need to
166+
// preserve the assignment either.
167+
TerminatorKind::Unreachable
168+
}
169+
Some(target) => {
170+
block.statements.push(Statement {
171+
source_info: terminator.source_info,
172+
kind: StatementKind::Assign(Box::new((
173+
*destination,
174+
Rvalue::Use(Operand::Copy(derefed_place)),
175+
))),
176+
});
177+
TerminatorKind::Goto { target }
178+
}
174179
}
175180
}
176181
sym::discriminant_value => {

‎library/core/src/intrinsics.rs

+5-9
Original file line numberDiff line numberDiff line change
@@ -2020,16 +2020,12 @@ extern "rust-intrinsic" {
20202020
#[rustc_safe_intrinsic]
20212021
pub fn saturating_sub<T: Copy>(a: T, b: T) -> T;
20222022

2023-
/// This is a *typed* read, `copy *p` in MIR.
2023+
/// This is an implementation detail of [`crate::ptr::read`] and should
2024+
/// not be used anywhere else. See its comments for why this exists.
20242025
///
2025-
/// The stabilized form of this intrinsic is [`crate::ptr::read`], so
2026-
/// that can be implemented without needing to do an *untyped* copy
2027-
/// via [`copy_nonoverlapping`], and thus can get proper metadata.
2028-
///
2029-
/// This intrinsic can *only* be called with a copy or move of a local.
2030-
/// (It allows neither constants nor projections.)
2031-
///
2032-
/// To avoid introducing any `noalias` requirements, it just takes a pointer.
2026+
/// This intrinsic can *only* be called where the argument is a local without
2027+
/// projections (`read_via_copy(p)`, not `read_via_copy(*p)`) so that it
2028+
/// trivially obeys runtime-MIR rules about derefs in operands.
20332029
#[cfg(not(bootstrap))]
20342030
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
20352031
pub fn read_via_copy<T>(p: *const T) -> T;

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

+5-3
Original file line numberDiff line numberDiff line change
@@ -1136,10 +1136,12 @@ pub const unsafe fn replace<T>(dst: *mut T, mut src: T) -> T {
11361136
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
11371137
pub const unsafe fn read<T>(src: *const T) -> T {
11381138
// It would be semantically correct to implement this via `copy_nonoverlapping`
1139-
// and `MaybeUninit`, as was done before PR #109035.
1139+
// and `MaybeUninit`, as was done before PR #109035. Calling `assume_init`
1140+
// provides enough information to know that this is a typed operation.
11401141

1141-
// However, it switched to intrinsic that lowers to `_0 = *src` in MIR in
1142-
// order to address a few implementation issues:
1142+
// However, as of March 2023 the compiler was not capable of taking advantage
1143+
// of that information. Thus the implementation here switched to an intrinsic,
1144+
// which lowers to `_0 = *src` in MIR, to address a few issues:
11431145
//
11441146
// - Using `MaybeUninit::assume_init` after a `copy_nonoverlapping` was not
11451147
// turning the untyped copy into a typed load. As such, the generated

‎tests/codegen/ptr-read-metadata.rs

+96
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
// compile-flags: -O -Z merge-functions=disabled
2+
// no-system-llvm
3+
// ignore-debug (the extra assertions get in the way)
4+
5+
#![crate_type = "lib"]
6+
7+
// Ensure that various forms of reading pointers correctly annotate the `load`s
8+
// with `!noundef` and `!range` metadata to enable extra optimization.
9+
10+
use std::mem::MaybeUninit;
11+
12+
// CHECK-LABEL: define noundef i8 @copy_byte(
13+
#[no_mangle]
14+
pub unsafe fn copy_byte(p: *const u8) -> u8 {
15+
// CHECK-NOT: load
16+
// CHECK: load i8, ptr %p, align 1
17+
// CHECK-SAME: !noundef !
18+
// CHECK-NOT: load
19+
*p
20+
}
21+
22+
// CHECK-LABEL: define noundef i8 @read_byte(
23+
#[no_mangle]
24+
pub unsafe fn read_byte(p: *const u8) -> u8 {
25+
// CHECK-NOT: load
26+
// CHECK: load i8, ptr %p, align 1
27+
// CHECK-SAME: !noundef !
28+
// CHECK-NOT: load
29+
p.read()
30+
}
31+
32+
// CHECK-LABEL: define i8 @read_byte_maybe_uninit(
33+
#[no_mangle]
34+
pub unsafe fn read_byte_maybe_uninit(p: *const MaybeUninit<u8>) -> MaybeUninit<u8> {
35+
// CHECK-NOT: load
36+
// CHECK: load i8, ptr %p, align 1
37+
// CHECK-NOT: noundef
38+
// CHECK-NOT: load
39+
p.read()
40+
}
41+
42+
// CHECK-LABEL: define noundef i8 @read_byte_assume_init(
43+
#[no_mangle]
44+
pub unsafe fn read_byte_assume_init(p: &MaybeUninit<u8>) -> u8 {
45+
// CHECK-NOT: load
46+
// CHECK: load i8, ptr %p, align 1
47+
// CHECK-SAME: !noundef !
48+
// CHECK-NOT: load
49+
p.assume_init_read()
50+
}
51+
52+
// CHECK-LABEL: define noundef i32 @copy_char(
53+
#[no_mangle]
54+
pub unsafe fn copy_char(p: *const char) -> char {
55+
// CHECK-NOT: load
56+
// CHECK: load i32, ptr %p
57+
// CHECK-SAME: !range ![[RANGE:[0-9]+]]
58+
// CHECK-SAME: !noundef !
59+
// CHECK-NOT: load
60+
*p
61+
}
62+
63+
// CHECK-LABEL: define noundef i32 @read_char(
64+
#[no_mangle]
65+
pub unsafe fn read_char(p: *const char) -> char {
66+
// CHECK-NOT: load
67+
// CHECK: load i32, ptr %p
68+
// CHECK-SAME: !range ![[RANGE]]
69+
// CHECK-SAME: !noundef !
70+
// CHECK-NOT: load
71+
p.read()
72+
}
73+
74+
// CHECK-LABEL: define i32 @read_char_maybe_uninit(
75+
#[no_mangle]
76+
pub unsafe fn read_char_maybe_uninit(p: *const MaybeUninit<char>) -> MaybeUninit<char> {
77+
// CHECK-NOT: load
78+
// CHECK: load i32, ptr %p
79+
// CHECK-NOT: range
80+
// CHECK-NOT: noundef
81+
// CHECK-NOT: load
82+
p.read()
83+
}
84+
85+
// CHECK-LABEL: define noundef i32 @read_char_assume_init(
86+
#[no_mangle]
87+
pub unsafe fn read_char_assume_init(p: &MaybeUninit<char>) -> char {
88+
// CHECK-NOT: load
89+
// CHECK: load i32, ptr %p
90+
// CHECK-SAME: !range ![[RANGE]]
91+
// CHECK-SAME: !noundef !
92+
// CHECK-NOT: load
93+
p.assume_init_read()
94+
}
95+
96+
// CHECK: ![[RANGE]] = !{i32 0, i32 1114112}

‎tests/codegen/read-noundef-metadata.rs

-51
This file was deleted.

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

-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
- // mir::Constant
1616
- // + span: $DIR/lower_intrinsics.rs:90:14: 90:45
1717
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Never) -> Never {read_via_copy::<Never>}, val: Value(<ZST>) }
18-
+ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
1918
+ unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
2019
}
2120
}

0 commit comments

Comments
 (0)