Add intrinsic for Option::Some(_) offset #109095

llogiq · 2023-03-13T20:40:16Z

This replaces the prior hacky calculation with an option_some_offset intrinsic that directly gets the offset from the type layout.

cc @scottmcm #108545

rustbot · 2023-03-13T20:40:24Z

r? @scottmcm

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

rustbot · 2023-03-13T20:40:28Z

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

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

oli-obk · 2023-03-13T21:15:10Z

library/core/src/intrinsics.rs

+
+#[cfg(not(bootstrap))]
+extern "rust-intrinsic" {
+    /// The offset of the `Some(_)` value of an `Option<T>`. Used internally for


Please re-add the FIXME from the hacky libstd impl here, so we remember to nuke this intrinsic in the future

the8472 · 2023-03-13T21:27:41Z

library/core/src/option.rs

+    /// As this version will only be ever used to compile rustc and the performance
+    /// penalty is negligible, use a minimal implementation here.
+    #[cfg(bootstrap)]
+    const SOME_BYTE_OFFSET_GUESS: usize = mem::size_of::<Option<T>>() - mem::size_of::<T>();


0 should be the safest choice here.

llogiq · 2023-03-14T07:41:14Z

I'm a bit confused about the error. I had included the const-unstable feature in library/core/src/lib.rs, but it appears to be required whenever the const is actually instantiated? That to me looks like a bug in the feature gate check. Should I simply make the intrinsic const-stable?

oli-obk · 2023-03-14T07:50:06Z

Should I simply make the intrinsic const-stable?

if the intrinsic is used from stable code, then yes, you need to make it stable.

Noratrieb · 2023-03-14T10:27:22Z

library/core/src/intrinsics.rs

@@ -2550,3 +2550,13 @@ pub const unsafe fn write_bytes<T>(dst: *mut T, val: u8, count: usize) {
        write_bytes(dst, val, count)
    }
 }
+
+#[cfg(not(bootstrap))]
+extern "rust-intrinsic" {


Can you add the intrinsics to the main extern "rust-intrinsic" block instead and just cfg the fn directly?

RalfJung · 2023-03-14T16:35:15Z

compiler/rustc_codegen_ssa/src/mir/intrinsic.rs

+                let FieldsShape::Arbitrary { offsets, .. } = &variant.fields else { bug!() };
+                bx.const_usize(offsets[0].bytes())


Why not use variant.fields.offset(0)? I am not sure if the bug! here is truly unreachable. Layouts can take very surprising shapes sometimes.

RalfJung · 2023-03-14T16:35:27Z

compiler/rustc_codegen_ssa/src/mir/intrinsic.rs

+            sym::option_some_offset => {
+                let ty = substs.type_at(0);
+                let layout = bx.layout_of(ty);
+                let Variants::Multiple { variants, .. } = layout.layout.variants() else { bug!() };


I think this will ICE on Option<!>.

Oh, a test for Option<Never> would be a good add, regardless of implementation 👍

RalfJung · 2023-03-14T16:36:02Z

compiler/rustc_const_eval/src/interpret/intrinsics.rs

+            let Variants::Multiple { variants, .. } = layout.layout.variants() else { bug!() };
+            let Some(variant) = variants.iter().last() else { bug!() };
+            let FieldsShape::Arbitrary { offsets, .. } = &variant.fields else { bug!() };
+            ConstValue::from_target_usize(offsets[0].bytes(), &tcx)


I think we should avoid duplicating this code.

Ok, where to put it then?

RalfJung · 2023-03-14T16:38:12Z

I'm a bit confused about the error. I had included the const-unstable feature in library/core/src/lib.rs, but it appears to be required whenever the const is actually instantiated? That to me looks like a bug in the feature gate check. Should I simply make the intrinsic const-stable?

Const stability checking is fully recursive, unlike the usual library stability checking. We want to avoid accidentally stably exposing the behavior of some unstable function / intrinsic at compile time.

scottmcm

I think this probably needs some updates before it can land, but I'm usually just a libs reviewer, so this should go to someone better equipped to evaluate the right way to do this in the compiler.

r? compiler

library/core/src/intrinsics.rs

llogiq · 2023-03-15T09:44:30Z

Closing this in favor of a offset_payload_ptr-intrinsic based solution.

…ce, r=eholk move Option::as_slice to intrinsic `@scottmcm` suggested on rust-lang#109095 I use a direct approach of unpacking the operation in MIR lowering, so here's the implementation. cc `@nikic` as this should hopefully unblock rust-lang#107224 (though perhaps other changes to the prior implementation, which I left for bootstrapping, are needed).

…ce, r=eholk move Option::as_slice to intrinsic ``@scottmcm`` suggested on rust-lang#109095 I use a direct approach of unpacking the operation in MIR lowering, so here's the implementation. cc ``@nikic`` as this should hopefully unblock rust-lang#107224 (though perhaps other changes to the prior implementation, which I left for bootstrapping, are needed).

…ce, r=eholk move Option::as_slice to intrinsic ```@scottmcm``` suggested on rust-lang#109095 I use a direct approach of unpacking the operation in MIR lowering, so here's the implementation. cc ```@nikic``` as this should hopefully unblock rust-lang#107224 (though perhaps other changes to the prior implementation, which I left for bootstrapping, are needed).

…ce, r=eholk move Option::as_slice to intrinsic ````@scottmcm```` suggested on rust-lang#109095 I use a direct approach of unpacking the operation in MIR lowering, so here's the implementation. cc ````@nikic```` as this should hopefully unblock rust-lang#107224 (though perhaps other changes to the prior implementation, which I left for bootstrapping, are needed).

rustbot assigned scottmcm Mar 13, 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 Mar 13, 2023

llogiq mentioned this pull request Mar 13, 2023

Tracking Issue for option_as_slice #108545

Closed

3 tasks

oli-obk reviewed Mar 13, 2023

View reviewed changes

the8472 reviewed Mar 13, 2023

View reviewed changes

Noratrieb reviewed Mar 14, 2023

View reviewed changes

llogiq force-pushed the option-some-offset-intrinsic branch from da1a88c to 86bc827 Compare March 14, 2023 12:24

RalfJung reviewed Mar 14, 2023

View reviewed changes

Add intrinsic for Option::Some(_) offset

a5ac44e

llogiq force-pushed the option-some-offset-intrinsic branch from 86bc827 to a5ac44e Compare March 14, 2023 17:13

scottmcm reviewed Mar 14, 2023

View reviewed changes

library/core/src/intrinsics.rs Show resolved Hide resolved

rustbot assigned michaelwoerister and petrochenkov and unassigned scottmcm Mar 14, 2023

scottmcm unassigned petrochenkov Mar 14, 2023

scottmcm mentioned this pull request Mar 14, 2023

Upgrade to LLVM 16 #107224

Merged

llogiq closed this Mar 15, 2023

llogiq deleted the option-some-offset-intrinsic branch March 15, 2023 09:44

llogiq mentioned this pull request Mar 15, 2023

move Option::as_slice to intrinsic #109179

Merged

michaelwoerister removed their assignment Mar 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add intrinsic for Option::Some(_) offset #109095

Add intrinsic for Option::Some(_) offset #109095

llogiq commented Mar 13, 2023 •

edited by scottmcm

Loading

rustbot commented Mar 13, 2023

rustbot commented Mar 13, 2023

oli-obk Mar 13, 2023

the8472 Mar 13, 2023

llogiq commented Mar 14, 2023

oli-obk commented Mar 14, 2023

Noratrieb Mar 14, 2023

RalfJung Mar 14, 2023

RalfJung Mar 14, 2023

scottmcm Mar 14, 2023

RalfJung Mar 14, 2023

llogiq Mar 14, 2023

RalfJung commented Mar 14, 2023

scottmcm left a comment

llogiq commented Mar 15, 2023

		let FieldsShape::Arbitrary { offsets, .. } = &variant.fields else { bug!() };
		bx.const_usize(offsets[0].bytes())

Add intrinsic for Option::Some(_) offset #109095

Add intrinsic for Option::Some(_) offset #109095

Conversation

llogiq commented Mar 13, 2023 • edited by scottmcm Loading

rustbot commented Mar 13, 2023

rustbot commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq commented Mar 14, 2023

oli-obk commented Mar 14, 2023

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

RalfJung commented Mar 14, 2023

scottmcm left a comment

Choose a reason for hiding this comment

llogiq commented Mar 15, 2023

llogiq commented Mar 13, 2023 •

edited by scottmcm

Loading