move Option::as_slice to intrinsic

[llogiq]

@scottmcm suggested on #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 #107224 (though perhaps other changes to the prior implementation, which I left for bootstrapping, are needed).

[rustbot]

r? @scottmcm

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

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

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

[llogiq]

No, rustbot, this is not for the libs team (although it touches on libs).

r? @rust-lang/compiler

[llogiq]

@scottmcm thanks for the help. I'll push the changes you proposed shortly once I have the test run complete.

[WaffleLapkin]

I don't think this should be done, a niche optimization doesn't justify a compiler intrinsic with all of its maintenance cost imo.

We should instead implement offset_of support for enums (btw is someone working on that? I could take a look if nobody is currently implementing this)

[llogiq]

I'm fine with extending offset_of!, too, even if unstably, but as that wasn't ready yet, and the current implementation blocks a LLVM upgrade, I suggested this as a stop-gap measure.

[oli-obk]

to unblock the LLVM upgrade we could simply change the body to

match self {
    None => &[],
    Some(x) => std::slice::from_ref(x),
}

Less efficient, but obviously correct

[JakobDegen]

I don't think this should be done, a niche optimization doesn't justify a compiler intrinsic with all of its maintenance cost imo.

Idk, an intrinsic that gets lowered in mir doesn't really have a lot of maintenance cost imo. Especially with something like this, which we'll reliably be able to support between changes to mir

[oli-obk]

The only reason not to do this and pick my inefficient version instead is that it serves as motivation to work on offset_of to support enum variants.

[llogiq]

Don't worry about that, @oli-obk, I'll extend offset_of! once it's ready and then remove the intrinsic. Edit: I hope that also alleviates @WaffleLapkin 's objections.

[llogiq]

I should however replace the old implementation with @oli-obk's proposal to ensure that the LLVM build is actually unblocked.

[WaffleLapkin]

I'll extend offset_of! once it's ready and then remove the intrinsic.

FYI: @drmeepster (author of #106934) had plans on doing this after the PR merges, don't forget to communicate with him, so the work is not duplicated :)

Edit: I hope that also alleviates WaffleLapkin's objections.

I still don't love the idea of adding an intrinsic, but I won't object to merging this in the meantime.

[llogiq]

@eholk do you have time to r+ this? Or should we ask for another reviewer?

[eholk]

The code looks pretty good to me. Is the main reason to do this to unblock the LLVM upgrade, or are there further benefits for it?

[llogiq]

That and the previous implementation was a hack that could result in suboptimal codegen if the type layout algorithm changed in a way that led to a different payload offset between Option<T> and Option<MaybeUninit<T>>. With this implementation that is no longer a concern.

Again, this is only a stop-gap until we can get the same effect via offset_of! as that implementation needs more work and then must still be extended to cover enums.

[llogiq]

@eholk do you need any further information to decide how to proceed?

[eholk]

Looks good to me!

@bors r+

[bors]

📌 Commit 27e9ee9 has been approved by eholk

It is now in the queue for this repository.