Allow `read`ing a `*mut` without an internal cast #111233

scottmcm · 2023-05-05T05:09:07Z

Continuing on from #110837 and #111010, another PR to

reduce your useless MIR with this small trick!

rustbot · 2023-05-05T05:09:15Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

scottmcm · 2023-05-05T05:10:53Z

tests/mir-opt/lower_intrinsics.read_via_copy_primitive.LowerIntrinsics.diff

-          _2 = &raw const (*_1);           // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47
-         _0 = read_via_copy::<i32>(move _2) -> [return: bb1, unwind unreachable]; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48
+          _2 = _1;                         // scope 1 at $DIR/lower_intrinsics.rs:+1:46: +1:47
+-         _0 = read_via_copy::<&i32, i32>(move _2) -> [return: bb1, unwind unreachable]; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48


Note that this also allows directly reading a &, though it's not used in library in this PR.

rustbot · 2023-05-05T07:37:01Z

The Miri subtree was changed

cc @rust-lang/miri

WaffleLapkin

This looks reasonable to me, but I'd like to see if we actually gain something tangible.

WaffleLapkin · 2023-05-05T08:17:52Z

tests/mir-opt/pre-codegen/mem_swap.swap_generic.PreCodegen.after.mir

+        StorageDead(_6);                 // scope 2 at $SRC_DIR/core/src/mem/mod.rs:LL:COL
+        StorageDead(_5);                 // scope 2 at $SRC_DIR/core/src/mem/mod.rs:LL:COL
+        StorageDead(_4);                 // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL
+        goto -> bb1;                     // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL


Not related to this PR; I'm just curious: why do we goto -> bb1 instead of simply return?

LLVM prefers having a single return terminator. So have a pass to replace such "go to return", but disabled by default. @saethlin did a perf run recently IIRC.

#106362 is the "just enable the pass" perf runs
#106550 is enabling the pass then cleaning up after it later in codegen

I don't think this is particularly worth pursuing based on the data available in those PRs, but I have also noticed that there are cases where we can end up with multiple return terminators heading into codegen. Perhaps that itself would be interesting to look into.

WaffleLapkin · 2023-05-05T08:22:34Z

tests/mir-opt/lower_intrinsics.ptr_offset_mut.LowerIntrinsics.diff

+-                                          // + span: $DIR/lower_intrinsics.rs:154:5: 154:29
+-                                          // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*mut i32, usize) -> *mut i32 {offset::<*mut i32, usize>}, val: Value(<ZST>) }
+         _0 = Offset(move _3, move _4);   // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35
+         goto -> bb1;                     // scope 0 at $DIR/lower_intrinsics.rs:+1:5: +1:35


Similarly to the other comment, why do we goto to bb1, where bb1 can be simply inlined?

Separation of concerns: LowerIntrinsics is just tasked with replacing the call terminator. Merging the blocks is left to SimplifyCfg pass.

WaffleLapkin · 2023-05-05T08:25:04Z

@bors try @rust-timer queue

bors · 2023-05-05T08:25:13Z

⌛ Trying commit 1464345 with merge 1abbc984fa15fd4b95f10542fb07d730767dcd77...

RalfJung · 2023-05-05T09:32:59Z

library/core/src/mem/mod.rs

    // SAFETY: We read from `dest` but directly write `src` into it afterwards,
    // such that the old value is not duplicated. Nothing is dropped and
    // nothing here can panic.
    unsafe {
-        let result = ptr::read(dest);
+        let result = ptr::read_mut(dest);


Compared to ptr::read, this just avoids a single MIR statement to turn the *mut into a *const, right? I am quite surprised that this would be a change worth doing. And if it is worth doing, a more holistic approach might be to just not do that cast and instead allow *mut everywhere that *const is allowed?

This does that a bit more for the method versions -- *mut has had read for 5 years now, and this PR makes that 1 statement shorter, which is certainly unimportant on its own but impacts every time it's mir-inlined. I suppose we can compatibly add impl Trait parameters to functions now, so we could consider letting ptr::read take more things too, but I don't know how disruptive that would be -- or if libs-api would even want it.

But I'm also not certain I understand what you were imagining with "allow *mut everywhere that *const is allowed". Could you sketch it out a bit more? I'm curious. (AFAIK MIR is typed enough that we can't omit these casts, even though they're NOPs?)

I was thinking MIR optimizations could remove the *mut to *const cast that was introduced by MIR building, and our notion of "well-typed MIR" could be weakened to say that this is fine.

bors · 2023-05-05T10:43:15Z

☀️ Try build successful - checks-actions
Build commit: 1abbc984fa15fd4b95f10542fb07d730767dcd77 (1abbc984fa15fd4b95f10542fb07d730767dcd77)

rust-timer · 2023-05-05T12:59:34Z

Finished benchmarking commit (1abbc984fa15fd4b95f10542fb07d730767dcd77): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.3%]	2
Regressions ❌ (secondary)	0.4%	[0.3%, 0.5%]	8
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-1.3%, 0.3%]	3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.5%	[1.5%, 1.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-4.2%, -2.1%]	3
Improvements ✅ (secondary)	-2.5%	[-2.8%, -2.2%]	3
All ❌✅ (primary)	-1.8%	[-4.2%, 1.5%]	4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 1.1%]	33
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	5
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	9
Improvements ✅ (secondary)	-0.8%	[-0.9%, -0.7%]	8
All ❌✅ (primary)	0.1%	[-0.2%, 1.1%]	42

Bootstrap: 654.604s -> 654.776s (0.03%)

rustbot assigned WaffleLapkin May 5, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 5, 2023

scottmcm commented May 5, 2023

View reviewed changes