-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Ensure ptr::read
gets all the same LLVM load
metadata that dereferencing does
#109035
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
b2c717f
0b96fee
1f70bb8
87696fd
e7c6ad8
dfc3377
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2020,6 +2020,20 @@ extern "rust-intrinsic" { | |||||||||||
#[rustc_safe_intrinsic] | ||||||||||||
pub fn saturating_sub<T: Copy>(a: T, b: T) -> T; | ||||||||||||
|
||||||||||||
/// This is a *typed* read, `copy *p` in MIR. | ||||||||||||
/// | ||||||||||||
/// The stabilized form of this intrinsic is [`crate::ptr::read`], so | ||||||||||||
/// that can be implemented without needing to do an *untyped* copy | ||||||||||||
/// via [`copy_nonoverlapping`], and thus can get proper metadata. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
/// | ||||||||||||
/// This intrinsic can *only* be called with a copy or move of a local. | ||||||||||||
/// (It allows neither constants nor projections.) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This confused me a bit — at first I though that Maybe the docs can be clarified a bit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a strange (and very syntactic) restriction? Isn't there a high risk that some other program transformation might, for instance, turn let x = s.f;
read_via_copy(x) into read_via_copy(s.f) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That can be fixed via introducing a temporary if it becomes a problem (and Jakob mentioned that on... Zulip?), but it seems the current tendency in the MIR is to aggressively desugar everything and introduce temporaries everywhere, then roll them back up in opt passes. If the implementation works and produces less MIR than the alternative, I think there's a merit in not introducing One More Temporary for the crab to claw through. Maybe we should note why this "bug" was not "fixed", though, so that if anyone comes by and it needs to be fixed, they can immediately change it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is about as big an optimization footgun as can exist. What saves us is that this runs before optimizations, so they don't have to care There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still seems rather fragile, and needs at least a comment explaining the situation. |
||||||||||||
/// | ||||||||||||
/// To avoid introducing any `noalias` requirements, it just takes a pointer. | ||||||||||||
#[cfg(not(bootstrap))] | ||||||||||||
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")] | ||||||||||||
pub fn read_via_copy<T>(p: *const T) -> T; | ||||||||||||
WaffleLapkin marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
|
||||||||||||
/// Returns the value of the discriminant for the variant in 'v'; | ||||||||||||
/// if `T` has no discriminant, returns `0`. | ||||||||||||
/// | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1135,27 +1135,56 @@ pub const unsafe fn replace<T>(dst: *mut T, mut src: T) -> T { | |
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")] | ||
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces | ||
pub const unsafe fn read<T>(src: *const T) -> T { | ||
// We are calling the intrinsics directly to avoid function calls in the generated code | ||
// as `intrinsics::copy_nonoverlapping` is a wrapper function. | ||
extern "rust-intrinsic" { | ||
#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")] | ||
fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize); | ||
} | ||
// It would be semantically correct to implement this via `copy_nonoverlapping` | ||
// and `MaybeUninit`, as was done before PR #109035. | ||
|
||
let mut tmp = MaybeUninit::<T>::uninit(); | ||
// SAFETY: the caller must guarantee that `src` is valid for reads. | ||
// `src` cannot overlap `tmp` because `tmp` was just allocated on | ||
// the stack as a separate allocated object. | ||
// However, it switched to intrinsic that lowers to `_0 = *src` in MIR in | ||
// order to address a few implementation issues: | ||
// | ||
// Also, since we just wrote a valid value into `tmp`, it is guaranteed | ||
// to be properly initialized. | ||
// - Using `MaybeUninit::assume_init` after a `copy_nonoverlapping` was not | ||
// turning the untyped copy into a typed load. As such, the generated | ||
// `load` in LLVM didn't get various metadata, such as `!range` (#73258), | ||
// `!nonnull`, and `!noundef`, resulting in poorer optimization. | ||
scottmcm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// - Going through the extra local resulted in multiple extra copies, even | ||
// in optimized MIR. (Ignoring StorageLive/Dead, the intrinsic is one | ||
// MIR statement, while the previous implementation was eight.) LLVM | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To show my work for these numbers, With this PR: bb0: {
StorageLive(_2); // scope 0 at C:\src\rust\tests\codegen\aaaaaa-mir-dump-demo.rs:9:5: 9:23
_0 = (*_1); // scope 2 at C:\src\rust\library\core\src\ptr\mod.rs:1167:13: 1167:50
StorageDead(_2); // scope 0 at C:\src\rust\tests\codegen\aaaaaa-mir-dump-demo.rs:9:5: 9:23
return; // scope 0 at C:\src\rust\tests\codegen\aaaaaa-mir-dump-demo.rs:10:2: 10:2
} Previously: bb0: {
StorageLive(_6); // scope 0 at C:\src\rust\tests\codegen\aaaaaa-mir-dump-demo.rs:12:5: 12:27
StorageLive(_2); // scope 2 at C:\src\rust\library\core\src\ptr\mod.rs:1195:17: 1195:24
StorageLive(_7); // scope 5 at C:\src\rust\library\core\src\mem\maybe_uninit.rs:314:31: 314:33
_2 = MaybeUninit::<T> { uninit: move _7 }; // scope 5 at C:\src\rust\library\core\src\mem\maybe_uninit.rs:314:9: 314:35
StorageDead(_7); // scope 5 at C:\src\rust\library\core\src\mem\maybe_uninit.rs:314:34: 314:35
StorageLive(_3); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1196:38: 1196:54
StorageLive(_4); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1196:38: 1196:54
_4 = &mut _2; // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1196:38: 1196:54
StorageLive(_8); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1196:42: 1196:54
_8 = &raw mut (*_4); // scope 6 at C:\src\rust\library\core\src\mem\maybe_uninit.rs:569:9: 569:13
_3 = _8 as *mut T (PtrToPtr); // scope 6 at C:\src\rust\library\core\src\mem\maybe_uninit.rs:569:9: 569:33
StorageDead(_8); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1196:42: 1196:54
StorageDead(_4); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1196:53: 1196:54
copy_nonoverlapping(dst = move _3, src = _1, count = const 1_usize); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1196:13: 1196:58
StorageDead(_3); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1196:57: 1196:58
StorageLive(_5); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1197:13: 1197:16
_5 = move _2; // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1197:13: 1197:16
StorageLive(_9); // scope 8 at C:\src\rust\library\core\src\mem\maybe_uninit.rs:627:38: 627:48
_9 = move (_5.1: std::mem::ManuallyDrop<T>); // scope 8 at C:\src\rust\library\core\src\mem\maybe_uninit.rs:627:38: 627:48
_0 = move (_9.0: T); // scope 9 at C:\src\rust\library\core\src\mem\manually_drop.rs:89:9: 89:19
StorageDead(_9); // scope 8 at C:\src\rust\library\core\src\mem\maybe_uninit.rs:627:48: 627:49
StorageDead(_5); // scope 3 at C:\src\rust\library\core\src\ptr\mod.rs:1197:29: 1197:30
StorageDead(_2); // scope 2 at C:\src\rust\library\core\src\ptr\mod.rs:1198:9: 1198:10
StorageDead(_6); // scope 0 at C:\src\rust\tests\codegen\aaaaaa-mir-dump-demo.rs:12:5: 12:27
return; // scope 0 at C:\src\rust\tests\codegen\aaaaaa-mir-dump-demo.rs:13:2: 13:2
} That's of course not a semantic problem, as both work fine, but I think it makes a nice demonstration of how this ends up being practically useful, especially in conjunction with MIR inlining. |
||
// could sometimes optimize them away, but because `read` is at the core | ||
// of so many things, not having them in the first place improves what we | ||
// hand off to the backend. For example, `mem::replace::<Big>` previously | ||
// emitted 4 `alloca` and 6 `memcpy`s, but is now 1 `alloc` and 3 `memcpy`s. | ||
// - In general, this approach keeps us from getting any more bugs (like | ||
// #106369) that boil down to "`read(p)` is worse than `*p`", as this | ||
// makes them look identical to the backend (or other MIR consumers). | ||
// | ||
// Future enhancements to MIR optimizations might well allow this to return | ||
// to the previous implementation, rather than using an intrinsic. | ||
workingjubilee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// SAFETY: the caller must guarantee that `src` is valid for reads. | ||
unsafe { | ||
assert_unsafe_precondition!( | ||
"ptr::read requires that the pointer argument is aligned and non-null", | ||
[T](src: *const T) => is_aligned_and_not_null(src) | ||
); | ||
copy_nonoverlapping(src, tmp.as_mut_ptr(), 1); | ||
tmp.assume_init() | ||
|
||
#[cfg(bootstrap)] | ||
{ | ||
// We are calling the intrinsics directly to avoid function calls in the | ||
// generated code as `intrinsics::copy_nonoverlapping` is a wrapper function. | ||
extern "rust-intrinsic" { | ||
#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")] | ||
fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize); | ||
} | ||
|
||
// `src` cannot overlap `tmp` because `tmp` was just allocated on | ||
// the stack as a separate allocated object. | ||
let mut tmp = MaybeUninit::<T>::uninit(); | ||
copy_nonoverlapping(src, tmp.as_mut_ptr(), 1); | ||
tmp.assume_init() | ||
} | ||
#[cfg(not(bootstrap))] | ||
{ | ||
crate::intrinsics::read_via_copy(src) | ||
} | ||
scottmcm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// compile-flags: -O | ||
// ignore-debug (the extra assertions get in the way) | ||
|
||
#![crate_type = "lib"] | ||
|
||
// From <https://github.com/rust-lang/rust/issues/106369#issuecomment-1369095304> | ||
|
||
// CHECK-LABEL: @issue_106369( | ||
#[no_mangle] | ||
pub unsafe fn issue_106369(ptr: *const &i32) -> bool { | ||
// CHECK-NOT: icmp | ||
// CHECK: ret i1 true | ||
// CHECK-NOT: icmp | ||
Some(std::ptr::read(ptr)).is_some() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
// compile-flags: -O | ||
// ignore-debug (the extra assertions get in the way) | ||
|
||
#![crate_type = "lib"] | ||
|
||
// Adapted from <https://github.com/rust-lang/rust/issues/73258#issue-637346014> | ||
|
||
#[derive(Clone, Copy)] | ||
#[repr(u8)] | ||
pub enum Foo { | ||
A, B, C, D, | ||
} | ||
|
||
// CHECK-LABEL: @issue_73258( | ||
#[no_mangle] | ||
pub unsafe fn issue_73258(ptr: *const Foo) -> Foo { | ||
// CHECK-NOT: icmp | ||
// CHECK-NOT: call | ||
// CHECK-NOT: br | ||
// CHECK-NOT: select | ||
|
||
// CHECK: %[[R:.+]] = load i8 | ||
// CHECK-SAME: !range ! | ||
|
||
// CHECK-NOT: icmp | ||
// CHECK-NOT: call | ||
// CHECK-NOT: br | ||
// CHECK-NOT: select | ||
|
||
// CHECK: ret i8 %[[R]] | ||
|
||
// CHECK-NOT: icmp | ||
// CHECK-NOT: call | ||
// CHECK-NOT: br | ||
// CHECK-NOT: select | ||
let k: Option<Foo> = Some(ptr.read()); | ||
return k.unwrap(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
// compile-flags: -O -Z merge-functions=disabled | ||
// no-system-llvm | ||
// ignore-debug (the extra assertions get in the way) | ||
|
||
#![crate_type = "lib"] | ||
|
||
// Ensure that various forms of reading pointers correctly annotate the `load`s | ||
// with `!noundef` metadata to enable extra optimization. The functions return | ||
// `MaybeUninit` to keep it from being inferred from the function type. | ||
scottmcm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
use std::mem::MaybeUninit; | ||
|
||
// CHECK-LABEL: define i8 @copy_byte( | ||
#[no_mangle] | ||
pub unsafe fn copy_byte(p: *const u8) -> MaybeUninit<u8> { | ||
// CHECK-NOT: load | ||
// CHECK: load i8, ptr %p, align 1 | ||
// CHECK-SAME: !noundef ! | ||
// CHECK-NOT: load | ||
MaybeUninit::new(*p) | ||
} | ||
|
||
// CHECK-LABEL: define i8 @read_byte( | ||
#[no_mangle] | ||
pub unsafe fn read_byte(p: *const u8) -> MaybeUninit<u8> { | ||
// CHECK-NOT: load | ||
// CHECK: load i8, ptr %p, align 1 | ||
// CHECK-SAME: !noundef ! | ||
// CHECK-NOT: load | ||
MaybeUninit::new(p.read()) | ||
} | ||
|
||
// CHECK-LABEL: define i8 @read_byte_maybe_uninit( | ||
#[no_mangle] | ||
pub unsafe fn read_byte_maybe_uninit(p: *const MaybeUninit<u8>) -> MaybeUninit<u8> { | ||
// CHECK-NOT: load | ||
// CHECK: load i8, ptr %p, align 1 | ||
// CHECK-NOT: noundef | ||
// CHECK-NOT: load | ||
p.read() | ||
} | ||
|
||
// CHECK-LABEL: define i8 @read_byte_assume_init( | ||
#[no_mangle] | ||
pub unsafe fn read_byte_assume_init(p: &MaybeUninit<u8>) -> MaybeUninit<u8> { | ||
// CHECK-NOT: load | ||
// CHECK: load i8, ptr %p, align 1 | ||
// CHECK-SAME: !noundef ! | ||
// CHECK-NOT: load | ||
MaybeUninit::new(p.assume_init_read()) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
- // MIR for `read_via_copy_primitive` before LowerIntrinsics | ||
+ // MIR for `read_via_copy_primitive` after LowerIntrinsics | ||
|
||
fn read_via_copy_primitive(_1: &i32) -> i32 { | ||
debug r => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:32: +0:33 | ||
let mut _0: i32; // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:44: +0:47 | ||
let mut _2: *const i32; // in scope 0 at $DIR/lower_intrinsics.rs:+1:46: +1:47 | ||
scope 1 { | ||
} | ||
|
||
bb0: { | ||
StorageLive(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47 | ||
_2 = &raw const (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47 | ||
- _0 = read_via_copy::<i32>(move _2) -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 | ||
- // mir::Constant | ||
- // + span: $DIR/lower_intrinsics.rs:85:14: 85:45 | ||
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const i32) -> i32 {read_via_copy::<i32>}, val: Value(<ZST>) } | ||
+ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 | ||
+ goto -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 | ||
} | ||
|
||
bb1: { | ||
StorageDead(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:47: +1:48 | ||
return; // scope 0 at $DIR/lower_intrinsics.rs:+2:2: +2:2 | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
- // MIR for `read_via_copy_uninhabited` before LowerIntrinsics | ||
+ // MIR for `read_via_copy_uninhabited` after LowerIntrinsics | ||
|
||
fn read_via_copy_uninhabited(_1: &Never) -> Never { | ||
debug r => _1; // in scope 0 at $DIR/lower_intrinsics.rs:+0:34: +0:35 | ||
let mut _0: Never; // return place in scope 0 at $DIR/lower_intrinsics.rs:+0:48: +0:53 | ||
let mut _2: *const Never; // in scope 0 at $DIR/lower_intrinsics.rs:+1:46: +1:47 | ||
scope 1 { | ||
} | ||
|
||
bb0: { | ||
StorageLive(_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47 | ||
_2 = &raw const (*_1); // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47 | ||
- _0 = read_via_copy::<Never>(move _2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 | ||
- // mir::Constant | ||
- // + span: $DIR/lower_intrinsics.rs:90:14: 90:45 | ||
- // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Never) -> Never {read_via_copy::<Never>}, val: Value(<ZST>) } | ||
+ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 | ||
+ unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 | ||
} | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.