Lower `intrinsics::offset` to `mir::BinOp::Offset` #110822

scottmcm · 2023-04-25T19:27:14Z

They're semantically the same, so this means the backends don't need to handle the intrinsic and means fewer MIR basic blocks in pointer arithmetic code.

rustbot · 2023-04-25T19:27:19Z

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-04-25T19:27:22Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

scottmcm · 2023-04-25T19:44:46Z

Oh, looks like ctfe doesn't handle the mir version? Will go deal with that.

@rustbot author

compiler/rustc_mir_transform/src/lower_intrinsics.rs

They're semantically the same, so this means the backends don't need to handle the intrinsic and means fewer MIR basic blocks in pointer arithmetic code.

rustbot · 2023-04-26T02:33:31Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

scottmcm · 2023-04-26T02:36:03Z

Ok, moved the offset code to binary_ptr_op instead of emulate_intrinsic.

@rustbot ready

compiler-errors

Oh, looks like ctfe doesn't handle the mir version? Will go deal with that.

Yeah, LowerIntrinsics gets run on runtime mir, so offset wouldn't show up in emulate_intrinsic.

Implementation looks fine, r=me nit or not

compiler/rustc_const_eval/src/const_eval/machine.rs

scottmcm · 2023-04-26T03:23:04Z

I liked the nit, but not with tidy forcing it to take a couple more lines, so I'll leave it with the nice clean diff. We'll change it to a match anyway should this ever start implementing more of them (like MIRI's implementation of this does).

@bors r=compiler-errors

bors · 2023-04-26T03:23:06Z

📌 Commit 05a665f has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-04-26T15:52:36Z

⌛ Testing commit 05a665f with merge 9c044d7...

bors · 2023-04-26T18:32:46Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 9c044d7 to master...

bors · 2023-04-26T18:32:46Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 9c044d7 to master...

rust-timer · 2023-04-26T19:50:17Z

Finished benchmarking commit (9c044d7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-9.7%	[-9.7%, -9.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-9.7%	[-9.7%, -9.7%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

RalfJung · 2023-04-27T10:21:38Z

compiler/rustc_const_eval/src/const_eval/machine.rs

+            let pointee_ty = left.layout.ty.builtin_deref(true).unwrap().ty;
+
+            let offset_ptr = ecx.ptr_offset_inbounds(ptr, pointee_ty, offset_count)?;
+            return Ok((Scalar::from_maybe_pointer(offset_ptr, ecx), false, left.layout.ty));


This indicates some opportunity for removing code duplication between CTFE and Miri.

…errors Use MIR's `Offset` for pointer `add` too ~~Status: draft while waiting for rust-lang#110822 to land, since this is built atop that.~~ ~~r? `@ghost~~` Canonical Rust code has mostly moved to `add`/`sub` on pointers, which take `usize`, instead of `offset` which takes `isize`. (And, relatedly, when `sub_ptr` was added it turned out it replaced every single in-tree use of `offset_from`, because `usize` is just so much more useful than `isize` in Rust.) Unfortunately, `intrinsics::offset` could only accept `*const` and `isize`, so there's a *huge* amount of type conversions back and forth being done. They're identity conversions in the backend, but still end up producing quite a lot of unhelpful MIR. This PR changes `intrinsics::offset` to accept `*const` *and* `*mut` along with `isize` *and* `usize`. Conveniently, the backends and CTFE already handle this, since MIR's `BinOp::Offset` [already supports all four combinations](https://github.com/rust-lang/rust/blob/adaac6b166df57ea5a20d56e4cce503b55aca927/compiler/rustc_const_eval/src/transform/validate.rs#L523-L528). To demonstrate the difference, I added some `mir-opt/pre-codegen/` tests around slice indexing. Here's the difference to `[T]::get_mut`, since it uses `<*mut _>::add` internally: ```diff `@@` -79,30 +70,21 `@@` fn slice_get_mut_usize(_1: &mut [u32], _2: usize) -> Option<&mut u32> { StorageLive(_12); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL StorageLive(_9); // scope 6 at $SRC_DIR/core/src/slice/index.rs:LL:COL _9 = _8 as *mut u32 (PtrToPtr); // scope 11 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - StorageLive(_13); // scope 13 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - _13 = _2 as isize (IntToInt); // scope 13 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - StorageLive(_14); // scope 15 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - StorageLive(_15); // scope 15 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - _15 = _9 as *const u32 (Pointer(MutToConstPointer)); // scope 15 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - _14 = Offset(move _15, _13); // scope 15 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - StorageDead(_15); // scope 15 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - _7 = move _14 as *mut u32 (PtrToPtr); // scope 15 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - StorageDead(_14); // scope 15 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL - StorageDead(_13); // scope 13 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL + _7 = Offset(_9, _2); // scope 13 at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL StorageDead(_9); // scope 6 at $SRC_DIR/core/src/slice/index.rs:LL:COL StorageDead(_12); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL StorageDead(_11); // scope 3 at $SRC_DIR/core/src/slice/index.rs:LL:COL ``` rust-lang@1c1c8e4#diff-a841b6a4538657add3f39bc895744331453d0625e7aace128b1f604f0b63c8fdR80

rustbot assigned compiler-errors Apr 25, 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. labels Apr 25, 2023

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2023

This comment has been minimized.

Sign in to view

deltragon reviewed Apr 25, 2023

View reviewed changes

compiler/rustc_mir_transform/src/lower_intrinsics.rs Outdated Show resolved Hide resolved

Lower intrinsics::offset to mir::BinOp::Offset

05a665f

They're semantically the same, so this means the backends don't need to handle the intrinsic and means fewer MIR basic blocks in pointer arithmetic code.

scottmcm force-pushed the lower-offset-to-mir branch from f9bb4a0 to 05a665f Compare April 26, 2023 02:33

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 26, 2023

compiler-errors approved these changes Apr 26, 2023

View reviewed changes

compiler/rustc_const_eval/src/const_eval/machine.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

scottmcm force-pushed the lower-offset-to-mir branch from afc7450 to 05a665f Compare April 26, 2023 03:21

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2023

scottmcm mentioned this pull request Apr 26, 2023

Use MIR's Offset for pointer add too #110837

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 26, 2023

bors merged commit 9c044d7 into rust-lang:master Apr 26, 2023

rustbot added this to the 1.71.0 milestone Apr 26, 2023

scottmcm deleted the lower-offset-to-mir branch April 26, 2023 18:34

RalfJung reviewed Apr 27, 2023

View reviewed changes

zhassan-aws mentioned this pull request May 18, 2023

Update rust toolchain to 2023-04-29 model-checking/kani#2452

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lower `intrinsics::offset` to `mir::BinOp::Offset` #110822

Lower `intrinsics::offset` to `mir::BinOp::Offset` #110822

scottmcm commented Apr 25, 2023

rustbot commented Apr 25, 2023

rustbot commented Apr 25, 2023

scottmcm commented Apr 25, 2023

This comment has been minimized.

rustbot commented Apr 26, 2023

scottmcm commented Apr 26, 2023

compiler-errors left a comment

This comment has been minimized.

scottmcm commented Apr 26, 2023 •

edited

Loading

bors commented Apr 26, 2023

bors commented Apr 26, 2023

bors commented Apr 26, 2023

bors commented Apr 26, 2023

rust-timer commented Apr 26, 2023

RalfJung Apr 27, 2023

Lower intrinsics::offset to mir::BinOp::Offset #110822

Lower intrinsics::offset to mir::BinOp::Offset #110822

Conversation

scottmcm commented Apr 25, 2023

rustbot commented Apr 25, 2023

rustbot commented Apr 25, 2023

scottmcm commented Apr 25, 2023

This comment has been minimized.

rustbot commented Apr 26, 2023

scottmcm commented Apr 26, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

This comment has been minimized.

scottmcm commented Apr 26, 2023 • edited Loading

bors commented Apr 26, 2023

bors commented Apr 26, 2023

bors commented Apr 26, 2023

bors commented Apr 26, 2023

rust-timer commented Apr 26, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

RalfJung Apr 27, 2023

Choose a reason for hiding this comment

Lower `intrinsics::offset` to `mir::BinOp::Offset` #110822

Lower `intrinsics::offset` to `mir::BinOp::Offset` #110822

scottmcm commented Apr 26, 2023 •

edited

Loading