move Option::as_slice to intrinsic by llogiq · Pull Request #109179 · rust-lang/rust

llogiq · 2023-03-15T17:40:11Z

@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 · 2023-03-15T17:40:18Z

r? @scottmcm

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

rustbot · 2023-03-15T17:40:22Z

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 · 2023-03-15T17:41:24Z

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

r? @rust-lang/compiler

library/core/src/option.rs

compiler/rustc_mir_transform/src/lower_intrinsics.rs

llogiq · 2023-03-15T18:57:28Z

@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)

compiler/rustc_hir_analysis/src/check/intrinsic.rs

llogiq · 2023-03-16T06:49:35Z

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 · 2023-03-16T07:30:23Z

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 · 2023-03-16T23:56:55Z

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 · 2023-03-17T06:08:19Z

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 · 2023-03-17T07:23:56Z

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 · 2023-03-17T12:48:43Z

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

WaffleLapkin · 2023-03-17T13:52:09Z

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 · 2023-03-17T22:03:34Z

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

eholk · 2023-03-18T05:30:01Z

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 · 2023-03-18T06:30:48Z

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 · 2023-03-20T18:20:57Z

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

eholk · 2023-03-20T22:56:07Z

Looks good to me!

@bors r+

bors · 2023-03-20T22:56:09Z

📌 Commit 27e9ee9 has been approved by eholk

It is now in the queue for this repository.

…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).

Rollup of 11 pull requests Successful merges: - rust-lang#100311 (Fix handling of trailing bare CR in str::lines) - rust-lang#108997 (Change text -> rust highlighting in sanitizer.md) - rust-lang#109179 (move Option::as_slice to intrinsic) - rust-lang#109187 (Render source page layout with Askama) - rust-lang#109280 (Remove `VecMap`) - rust-lang#109295 (refactor `fn bootstrap::builder::Builder::compiler_for` logic) - rust-lang#109312 (rustdoc: Cleanup parent module tracking for doc links) - rust-lang#109317 (Update links for custom discriminants.) - rust-lang#109405 (RPITITs are `DefKind::Opaque` with new lowering strategy) - rust-lang#109414 (Do not consider synthesized RPITITs on missing items checks) - rust-lang#109435 (Detect uninhabited types early in const eval) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…ix, r=scottmcm Option::as_slice: fix comment The implementation got changed in rust-lang#117525 without updating the comment. Now the comment makes no sense any more since there is no intrinsic that returns a pointer in this function any more. (It is also very strange to say "in the new version" in a comment -- what is that supposed to tell someone reading the code 2 years later? That wording was introduced even earlier, in rust-lang#109179.) Cc `@GKFX` `@petrochenkov` `@llogiq` `@scottmcm`

Rollup merge of #144230 - RalfJung:option-as-slice-comment-fix, r=scottmcm Option::as_slice: fix comment The implementation got changed in #117525 without updating the comment. Now the comment makes no sense any more since there is no intrinsic that returns a pointer in this function any more. (It is also very strange to say "in the new version" in a comment -- what is that supposed to tell someone reading the code 2 years later? That wording was introduced even earlier, in #109179.) Cc `@GKFX` `@petrochenkov` `@llogiq` `@scottmcm`

…ix, r=scottmcm Option::as_slice: fix comment The implementation got changed in rust-lang#117525 without updating the comment. Now the comment makes no sense any more since there is no intrinsic that returns a pointer in this function any more. (It is also very strange to say "in the new version" in a comment -- what is that supposed to tell someone reading the code 2 years later? That wording was introduced even earlier, in rust-lang#109179.) Cc `@GKFX` `@petrochenkov` `@llogiq` `@scottmcm`

…ttmcm Option::as_slice: fix comment The implementation got changed in rust-lang/rust#117525 without updating the comment. Now the comment makes no sense any more since there is no intrinsic that returns a pointer in this function any more. (It is also very strange to say "in the new version" in a comment -- what is that supposed to tell someone reading the code 2 years later? That wording was introduced even earlier, in rust-lang/rust#109179.) Cc `@GKFX` `@petrochenkov` `@llogiq` `@scottmcm`

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

rustbot assigned eholk and unassigned scottmcm Mar 15, 2023

scottmcm reviewed Mar 15, 2023

View reviewed changes

library/core/src/option.rs Outdated Show resolved Hide resolved

scottmcm reviewed Mar 15, 2023

View reviewed changes

library/core/src/option.rs Outdated Show resolved Hide resolved

scottmcm reviewed Mar 15, 2023

View reviewed changes

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

WaffleLapkin reviewed Mar 15, 2023

View reviewed changes

compiler/rustc_hir_analysis/src/check/intrinsic.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_analysis/src/check/intrinsic.rs Outdated Show resolved Hide resolved

llogiq force-pushed the intrinsically-option-as-slice branch from e552c50 to 70da052 Compare March 15, 2023 22:16

This comment has been minimized.

Sign in to view

llogiq force-pushed the intrinsically-option-as-slice branch 2 times, most recently from 1e7f303 to 50846d5 Compare March 16, 2023 18:30

This comment has been minimized.

Sign in to view

llogiq force-pushed the intrinsically-option-as-slice branch from 50846d5 to a2d33d4 Compare March 17, 2023 06:55

llogiq force-pushed the intrinsically-option-as-slice branch from a2d33d4 to 6fa28de Compare March 17, 2023 16:01

the8472 mentioned this pull request Mar 17, 2023

Permit the MIR inliner to inline diverging functions #106428

Merged

move Option::as_slice to intrinsic

27e9ee9

llogiq force-pushed the intrinsically-option-as-slice branch from 6fa28de to 27e9ee9 Compare March 18, 2023 06:18

llogiq mentioned this pull request Mar 19, 2023

Tracking Issue for option_as_slice #108545

Closed

3 tasks

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 Mar 20, 2023

Dylan-DPC mentioned this pull request Mar 21, 2023

Rollup of 10 pull requests #109428

Closed

matthiaskrgr mentioned this pull request Mar 21, 2023

Rollup of 9 pull requests #109465

Closed

Dylan-DPC mentioned this pull request Mar 22, 2023

Rollup of 12 pull requests #109490

Closed

Dylan-DPC mentioned this pull request Mar 22, 2023

Rollup of 11 pull requests #109496

Merged

bors merged commit 14d0646 into rust-lang:master Mar 22, 2023

rustbot added this to the 1.70.0 milestone Mar 22, 2023

llogiq deleted the intrinsically-option-as-slice branch March 22, 2023 22:55

RalfJung mentioned this pull request Jul 20, 2025

Option::as_slice: fix comment #144230

Merged

Uh oh!

Conversation

llogiq commented Mar 15, 2023

Uh oh!

rustbot commented Mar 15, 2023

Uh oh!

rustbot commented Mar 15, 2023

Uh oh!

llogiq commented Mar 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llogiq commented Mar 15, 2023

Uh oh!

WaffleLapkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

llogiq commented Mar 16, 2023

Uh oh!

oli-obk commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

JakobDegen commented Mar 16, 2023

Uh oh!

oli-obk commented Mar 17, 2023

Uh oh!

llogiq commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llogiq commented Mar 17, 2023

Uh oh!

WaffleLapkin commented Mar 17, 2023

Uh oh!

llogiq commented Mar 17, 2023

Uh oh!

eholk commented Mar 18, 2023

Uh oh!

llogiq commented Mar 18, 2023

Uh oh!

llogiq commented Mar 20, 2023

Uh oh!

eholk commented Mar 20, 2023

Uh oh!

bors commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

oli-obk commented Mar 16, 2023 •

edited

Loading

llogiq commented Mar 17, 2023 •

edited

Loading