Add an intrinsic for `ptr::from_raw_parts(_mut)` #123840

scottmcm · 2024-04-12T03:40:48Z

Fixes #123174
cc @CAD97 @saethlin
r? @cjgillot

As suggested in #123190 (comment), this adds a new AggregateKind::RawPtr for creating a pointer from its data pointer and its metadata.

That means that slice::from_raw_parts and friends no longer need to hard-code pointer layout into libcore, and because it no longer does union hacks the MIR is shorter and more amenable to optimizations.

rustbot · 2024-04-12T03:40:57Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

scottmcm · 2024-04-12T03:46:04Z

tests/mir-opt/pre-codegen/vec_deref.vec_deref_to_slice.PreCodegen.after.panic-abort.mir

+        _5 = ((*_1).1: usize);
+        StorageLive(_6);
+        _6 = _4 as *const () (PtrToPtr);
+        _7 = *const [u8] from (_6, _5);


For comparison, on nightly this line is

_9 = std::ptr::metadata::PtrComponents::<[u8]> { data_pointer: _7, metadata: _3 }; _8 = std::ptr::metadata::PtrRepr::<[u8]> { const_ptr: move _9 }; _6 = (_8.0: *const [u8]);

if you put the threshold high enough to see it https://rust.godbolt.org/z/baT3bqEze

scottmcm · 2024-04-12T03:56:10Z

library/core/src/ptr/metadata.rs

+        // SAFETY: Accessing the value from the `PtrRepr` union is safe since *const T
+        // and PtrComponents<T> have the same memory layouts. Only std can make this
+        // guarantee.
+        unsafe { PtrRepr { components: PtrComponents { data_pointer, metadata } }.const_ptr }


Sadly another PR to add an intrinsic for https://doc.rust-lang.org/nightly/std/ptr/fn.metadata.html will be needed before this can delete PtrRepr and PtrComponents. But the scroll bar was long enough already that I didn't want to do that in this change.

scottmcm · 2024-04-12T04:01:08Z

compiler/rustc_mir_transform/src/gvn.rs

@@ -910,6 +911,8 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
            }
            // Do not track unions.
            AggregateKind::Adt(_, _, _, _, Some(_)) => return None,
+            // FIXME: Do the extra work to GVN `from_raw_parts`
+            AggregateKind::RawPtr(..) => return None,


I started down the path of an AggregateTy::RawPtr(ty), then decided that since GVN couldn't track the old code because of the union, I didn't need to shave this yak too right now.

rustbot · 2024-04-12T04:47:09Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

compiler/rustc_const_eval/src/interpret/step.rs

compiler/rustc_const_eval/src/interpret/operand.rs

scottmcm · 2024-04-12T17:15:43Z

Pulling something out of a review comment to provide some context:

and RawPtr also works for thin pointers it seems

Yeah, since ptr::from_raw_parts might not be inlined it needed to handle the unit-metadata cases too -- or at least I thought that was better than trying to do it with specialization in libcore.

InstSimplify will replace it with just a PtrToPtr cast if it does inline enough to be able to see that the metadata is (), though.

cjgillot

Thanks for investigating this @scottmcm!

cjgillot · 2024-04-13T01:47:59Z

compiler/rustc_borrowck/src/type_check/mod.rs

+            // raw pointer rvalues are checked in the MIR validator,
+            // and since they only deal in things that have no lifetimes,
+            // there's nothing that needs checking here.
+            return;


If RawPtr is only created by an intrinsic, this should bug!.

compiler/rustc_const_eval/src/transform/validate.rs

cjgillot · 2024-04-13T01:52:35Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

@@ -717,6 +717,24 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                OperandRef { val: OperandValue::Immediate(static_), layout }
            }
            mir::Rvalue::Use(ref operand) => self.codegen_operand(bx, operand),
+            mir::Rvalue::Aggregate(box mir::AggregateKind::RawPtr(..), ref fields) => {


Nit: should this appear in an earlier commit?

The first few commits here aren't split up super great, just checkpoints where it built and the tests pass. The tests "passing" for them wasn't terribly meaningful, though, since without an intrinsic for it and without the lowering using it, then the code wasn't actually ever running.

LMK if you're fine leaving it as-is or would rather I just merge the first few.

cjgillot · 2024-04-13T01:53:51Z

compiler/rustc_mir_transform/src/instsimplify.rs

@@ -36,6 +36,7 @@ impl<'tcx> MirPass<'tcx> for InstSimplify {
                        ctx.simplify_bool_cmp(&statement.source_info, rvalue);
                        ctx.simplify_ref_deref(&statement.source_info, rvalue);
                        ctx.simplify_len(&statement.source_info, rvalue);
+                        ctx.simplify_ptr_aggregate(&statement.source_info, rvalue);


Could the new opt be in a standalone commit, to see its effect on tests?

Added a new mir-opt test for byte_add on pointers to show what this does: https://github.com/rust-lang/rust/pull/123840/files#diff-89ce90739dcfc9de9ddd90700c234f28642574e2baad03a1bd1b0b6fee9e4016

cjgillot · 2024-04-13T01:59:16Z

compiler/rustc_middle/src/mir/syntax.rs

+    /// *Both* operands must always be included, even the unit value if this is
+    /// creating a thin pointer. If you're just converting between thin pointers,
+    /// you may want an [`Rvalue::Cast`] with [`CastKind::PtrToPtr`] instead.
+    RawPtr(Ty<'tcx>, Mutability),


In terms of generality, should this support only raw pointers, or borrows too? Or to be left as a follow-up experiment?

Right now I think that Rvalue::Ref is the only thing that makes references (well, I guess Transmutes too), so I'd rather leave it on just raw pointers, for now, with the &*_2 dance for turning it back into a reference. Especially since I don't want to worry about making sure that whatever SB/TB tagging is needed is emitting in Miri for things.

It's also nice in the library for the intrinsic to be able to be safe, which it can't if it makes references. That could be done with separate intrinsics so the raw pointer one can still be safe, but that's again making the size of the change bigger again, so I think not doing it in this PR, at least, is best.

cjgillot · 2024-04-13T02:00:05Z

compiler/rustc_mir_transform/src/gvn.rs

@@ -910,6 +911,8 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
            }
            // Do not track unions.
            AggregateKind::Adt(_, _, _, _, Some(_)) => return None,
+            // FIXME: Do the extra work to GVN `from_raw_parts`
+            AggregateKind::RawPtr(..) => return None,


scottmcm · 2024-04-14T23:39:07Z

Sorry for all the pushes, @cjgillot . Here's the diff since your review: https://github.com/rust-lang/rust/compare/70dee04ff7a06bebf69a63a373ee1f046e6b5878..1712f5c9aa7060425ed534f7d062992ab4055971

scottmcm · 2024-04-14T23:53:51Z

tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_thin.PreCodegen.after.panic-unwind.mir

+        StorageDead(_3);
+        StorageLive(_5);
+        _5 = _4 as *const () (PtrToPtr);
+        _0 = _4 as *const u32 (PtrToPtr);


As a serendipitous bonus, this PR means that using byte_add on a thin pointer is no longer worse in MIR than casting to *const u8 and offsetting yourself, as with unit metadata there's no longer any union dances needed, just the straight-forward casting.

Oh, even better -- with #123949 it also drops the irrelevant cast to *const ().

saethlin · 2024-04-15T02:25:51Z

library/core/src/intrinsics.rs

+#[rustc_intrinsic_must_be_overridden]
+#[cfg(not(bootstrap))]
+pub const fn aggregate_raw_ptr<P: AggregateRawPtr<D, Metadata = M>, D, M>(_data: D, _meta: M) -> P {
+    // No fallback because `libcore` doesn't want to know the layout


Suggested change

// No fallback because `libcore` doesn't want to know the layout

// To implement a fallback we'd have to assume the layout, but the whole point of this intrinsic is to not do that.

We do want to know the layout, we just can't :p

saethlin · 2024-04-15T02:27:52Z

compiler/rustc_mir_transform/src/instsimplify.rs

@@ -147,6 +157,30 @@ impl<'tcx> InstSimplifyContext<'tcx, '_> {
        }
    }

+    /// Transform "Aggregate(RawPtr, \[p, ()\])" ==> "Cast(PtrToPtr, p)".


This escaping is really unfortunate. Is it possible to use backticks for code formatting instead?

Sure. I was copying the other two method headers that also used ", so I've updated all three of them to backticks.

scottmcm · 2024-04-21T18:35:43Z

@bors r=cjgillot

bors · 2024-04-21T18:35:46Z

📌 Commit 5e785b1 has been approved by cjgillot

It is now in the queue for this repository.

…cjgillot Add an intrinsic for `ptr::from_raw_parts(_mut)` Fixes rust-lang#123174 cc `@CAD97` `@saethlin` r? `@cjgillot` As suggested in rust-lang#123190 (comment), this adds a new `AggregateKind::RawPtr` for creating a pointer from its data pointer and its metadata. That means that `slice::from_raw_parts` and friends no longer need to hard-code pointer layout into `libcore`, and because it no longer does union hacks the MIR is shorter and more amenable to optimizations.

matthiaskrgr · 2024-04-21T19:33:31Z

@bors r-
#124237 (comment)
proly fixes the ice? could you add a test?

scottmcm · 2024-04-21T19:37:01Z

Huh, curious that it still passes CI. I'll take a look.

matthiaskrgr · 2024-04-21T19:55:05Z

ah must have been a different PR, the rollup without this one also failed, but the passing test was something with RefTarget which threw me off 😅

@bors r=cjgillot

bors · 2024-04-21T19:55:08Z

📌 Commit 5e785b1 has been approved by cjgillot

It is now in the queue for this repository.

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#123840 (Add an intrinsic for `ptr::from_raw_parts(_mut)`) - rust-lang#124224 (cleanup: unnecessary clone during lower generics args) - rust-lang#124229 (Add gnullvm targets to manifest) - rust-lang#124231 (remove from reviewers) - rust-lang#124235 (Move some tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123840 - scottmcm:aggregate-kind-rawptr, r=cjgillot Add an intrinsic for `ptr::from_raw_parts(_mut)` Fixes rust-lang#123174 cc `@CAD97` `@saethlin` r? `@cjgillot` As suggested in rust-lang#123190 (comment), this adds a new `AggregateKind::RawPtr` for creating a pointer from its data pointer and its metadata. That means that `slice::from_raw_parts` and friends no longer need to hard-code pointer layout into `libcore`, and because it no longer does union hacks the MIR is shorter and more amenable to optimizations.

rustbot assigned cjgillot Apr 12, 2024

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 Apr 12, 2024

scottmcm commented Apr 12, 2024

View reviewed changes

scottmcm mentioned this pull request Apr 12, 2024

Shrink from_raw_parts's MIR so that Vec::deref MIR-inlines again #123190

Closed

scottmcm commented Apr 12, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

scottmcm commented Apr 12, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

scottmcm force-pushed the aggregate-kind-rawptr branch from 3cf58a9 to aa35d1b Compare April 12, 2024 05:01

This comment has been minimized.

Sign in to view

scottmcm force-pushed the aggregate-kind-rawptr branch from aa35d1b to c79c53a Compare April 12, 2024 05:28

RalfJung reviewed Apr 12, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/step.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 12, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/operand.rs Outdated Show resolved Hide resolved

scottmcm force-pushed the aggregate-kind-rawptr branch from c105f86 to ada5a3d Compare April 12, 2024 17:45

This comment has been minimized.

Sign in to view

scottmcm force-pushed the aggregate-kind-rawptr branch from ada5a3d to 70dee04 Compare April 12, 2024 18:05

cjgillot reviewed Apr 13, 2024

View reviewed changes

scottmcm force-pushed the aggregate-kind-rawptr branch 3 times, most recently from bf66fec to 1712f5c Compare April 14, 2024 19:48

scottmcm commented Apr 14, 2024

View reviewed changes

scottmcm mentioned this pull request Apr 14, 2024

UB Check blocks MIR inlining of Vec::deref #123174

Closed

saethlin reviewed Apr 15, 2024

View reviewed changes

scottmcm added 6 commits April 21, 2024 11:08

Add an intrinsic that lowers to AggregateKind::RawPtr

4f44426

Use it in the library, and InstSimplify it away in the easy places

de64ff7

InstSimplify from_raw_parts(p, ()) → p as _

9520ceb

Also handle AggregateKind::RawPtr in cg_cranelift

5e1d16c

Address PR feedback

bb8d6f7

Address more PR feedback

1398fe7

scottmcm force-pushed the aggregate-kind-rawptr branch from 4d71b43 to 1398fe7 Compare April 21, 2024 18:21

Update tests after 123949

5e785b1

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 21, 2024

matthiaskrgr mentioned this pull request Apr 21, 2024

Rollup of 5 pull requests #124237

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 21, 2024

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2024

matthiaskrgr mentioned this pull request Apr 21, 2024

Rollup of 5 pull requests #124241

Merged

bors merged commit 67872e7 into rust-lang:master Apr 21, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Apr 21, 2024

scottmcm deleted the aggregate-kind-rawptr branch April 21, 2024 23:14

scottmcm mentioned this pull request Apr 22, 2024

Add an intrinsic for ptr::metadata #124251

Draft

bjorn3 mentioned this pull request Apr 22, 2024

ICE: thin_box: RawPtr operands CValue(ByVal(v3)... rust-lang/rustc_codegen_cranelift#1485

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an intrinsic for `ptr::from_raw_parts(_mut)` #123840

Add an intrinsic for `ptr::from_raw_parts(_mut)` #123840

scottmcm commented Apr 12, 2024

rustbot commented Apr 12, 2024

scottmcm Apr 12, 2024 •

edited

scottmcm Apr 12, 2024

This comment has been minimized.

scottmcm Apr 12, 2024

cjgillot Apr 13, 2024

rustbot commented Apr 12, 2024

This comment has been minimized.

This comment has been minimized.

scottmcm commented Apr 12, 2024

This comment has been minimized.

cjgillot left a comment

cjgillot Apr 13, 2024

cjgillot Apr 13, 2024

scottmcm Apr 14, 2024

cjgillot Apr 13, 2024

scottmcm Apr 14, 2024

cjgillot Apr 13, 2024

scottmcm Apr 14, 2024

cjgillot Apr 13, 2024

scottmcm commented Apr 14, 2024

scottmcm Apr 14, 2024

scottmcm Apr 21, 2024

saethlin Apr 15, 2024

saethlin Apr 15, 2024

scottmcm Apr 17, 2024

scottmcm commented Apr 21, 2024

bors commented Apr 21, 2024

matthiaskrgr commented Apr 21, 2024

scottmcm commented Apr 21, 2024

matthiaskrgr commented Apr 21, 2024

bors commented Apr 21, 2024

	// No fallback because `libcore` doesn't want to know the layout
	// To implement a fallback we'd have to assume the layout, but the whole point of this intrinsic is to not do that.

Add an intrinsic for ptr::from_raw_parts(_mut) #123840

Add an intrinsic for ptr::from_raw_parts(_mut) #123840

Conversation

scottmcm commented Apr 12, 2024

rustbot commented Apr 12, 2024

scottmcm Apr 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Apr 12, 2024

This comment has been minimized.

This comment has been minimized.

scottmcm commented Apr 12, 2024

This comment has been minimized.

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Apr 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Apr 21, 2024

bors commented Apr 21, 2024

matthiaskrgr commented Apr 21, 2024

scottmcm commented Apr 21, 2024

matthiaskrgr commented Apr 21, 2024

bors commented Apr 21, 2024

Add an intrinsic for `ptr::from_raw_parts(_mut)` #123840

Add an intrinsic for `ptr::from_raw_parts(_mut)` #123840

scottmcm Apr 12, 2024 •

edited