Allow reifying intrinsics to `fn` pointers. #86699

eddyb · 2021-06-28T19:48:08Z

The main change is in ty/instance.rs, which is to simply have resolve_for_fn_ptr handle InstanceDef::Intrinsic the same way it does the other existing cases where a shim is needed to be able to have a fn pointer in the first place (virtual methods i.e. <dyn Trait as Trait>::method, and #[track_caller] functions).

However, there is another important change, which is that unlike other functions, intrinsics have their type-level ABI change from the extern "rust-intrinsic" fn "pseudo-ABI" to the normal Rust ABI.
This is possible because pseudo-ABIs like extern "rust-intrinsic" do not actually change the call ABI, but rather are used as a side-channel (instead of a more principled solution, like some #[rustc_intrinsic] attribute).

What this all means for the user is that they can/have to do transmute as unsafe fn(T) -> U, without the fact that the function was an intrinsic showing up in the typesystem at all.

That type-level ABI aspect also ties in with the motivation for finally fixing this, which is #84297, where ptr::{copy,copy_nonoverlapping} becoming stable reexports of the intrinsics in #81238 was backwards incompatible - with this PR, that shouldn't be an issue anymore.

(I also have a revert for #86003 ready - at least the parts not reverted already by e.g. #86295, if this PR is accepted; cc @pnkfelix)

In terms of this capability being insta-stable, we still get to control that via which intrinsics we mark stable.
The only one I could find right now is mem::transmute (and I've added a test that its size check still works when going through a fn pointer), and if we do the above mentioned #86003 revert, we'd also have ptr::{copy,copy_nonoverlapping}.

rust-highfive · 2021-06-28T19:48:12Z

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

nagisa · 2021-06-28T20:31:56Z

The implementation seems reasonable enough to me.

tmiasko · 2021-06-29T16:22:57Z

The intrinsics lowering pass lowers some of intrinsics calls into MIR statements. Given that pass runs unconditionally for user constructed MIR, and coercing intrinsics into function pointers was so far impossible, the implementation of lowered intrinsics was removed from both codegen and interpreter. This will have to change now, possibly by reintroducing removed implementations or by running the pass for shims.

eddyb · 2021-06-29T16:30:54Z

The DRY thing to do would be to run the pass for shims, I suppose, but thanks for pointing that out, I wasn't aware such a pass got added.

I'm not sure it's a priority, given the dearth of stably exposed intrinsics, and the fact that reification is a somewhat intentional action, but I will look into it if it's blocking landing this PR.

(For the record, I'd rather have codegen ICEs, or working reification, than split up intrinsics into reifiable vs not, because it forces us to not add/modify intrinsics in a way that is incompatible with reification)

apiraino · 2021-07-08T15:32:37Z

Issue was discussed today during T-compiler meeting (on Zulip)

@rustbot label -I-nominated

nikomatsakis · 2021-07-09T10:06:18Z

I'm tagging this as T-lang and nominating, as it seems like a lang change. It'd perhaps be nice to write up a lang proposal or something that summarizes it. It's a short PR. The details are:

The type of an intrinsic like transmute is a zero-sized function type.
Under this PR, it can be coerce to an extern "Rust" function pointer
This will create a wrapper around the intrinsic.
This mechanism is 'insta-stable', but the only stable intrinsic is believed to be transmute.

eddyb · 2021-07-09T10:19:18Z

but the only stable intrinsic is believed to be transmute

Note to self: still need to check whether stdarch contains any exposed intrinsics. They wouldn't have showed up in my initial search because of the submodule situation (which I had forgotten about).

bjorn3 · 2021-07-09T10:27:03Z

The simd_llvm module is not exported. Marking all of it's platform-intrinsics as pub(crate) didn't result in any compilation error, so none should be exported at the moment.

apiraino · 2021-07-15T10:28:47Z

Discussed in T-compiler meeting (Zulip thread)

@rustbot label -I-nominated

eddyb · 2021-07-15T10:34:01Z

@apiraino that was already handled in #86699 (comment) - the current nomination was for lang team. I will remove T-compiler to reduce confusion.

eddyb · 2021-07-20T16:35:49Z

The simd_llvm module is not exported. Marking all of it's platform-intrinsics as pub(crate) didn't result in any compilation error, so none should be exported at the moment.

Thanks for the help with this! On top of all that, the module doesn't have a stability attribute, and so I believe it will inherit this from library/core/src/lib.rs:

#[unstable(feature = "stdsimd", issue = "48556")]
mod core_arch;

Also, I just re-checked everything else, and it does seem like transmute is indeed the only (rust|platform)-intrinsic stable fn at this point in time.

bjorn3 · 2021-07-20T17:07:46Z

To prevent this PR from being insta-stable, maybe the function pointer cast could be feature-gated for all intrinsics or prevented just for transmute?

eddyb · 2021-07-20T17:15:38Z

That might be possible, though it would defeat the goal of being able to revert #86003, and publicly expose those intrinsics.

bjorn3 · 2021-07-20T17:20:37Z

In case of preventing it just for transmute it is still possible to revert #86003. If this PR has to be reverted for whatever reason it is still possible to re-introduce the wrappers for those intrinsics. It isn't possible to introduce a wrapper for transmute if it were to be stabily able to cast to a function pointer.

joshtriplett · 2021-07-20T17:57:33Z

We discussed this in the @rust-lang/lang meeting today, and we have no objections to the compiler team resolving this issue as planned. Our understanding is that the plan is to make intrinsics usable as if they were functions, and we agree with that plan. We don't have any concerns about however the compiler team plans to make that work.

Let's use an FCP to confirm that:

@rfcbot merge

rfcbot · 2021-07-20T17:57:34Z

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot · 2021-07-30T21:30:42Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

bors · 2021-08-05T00:08:33Z

☔ The latest upstream changes (presumably #86155) made this pull request unmergeable. Please resolve the merge conflicts.

apiraino · 2021-08-05T15:08:37Z

@rustbot label -to-announce

Avoid using the `copy_nonoverlapping` wrapper through `mem::replace`. This is a much simpler way to achieve the pre-rust-lang#86003 behavior of `mem::replace` not needing dynamically-sized `memcpy`s (at least before inlining), than re-doing rust-lang#81238 (which needs rust-lang#86699 or something similar). I didn't notice it until recently, but `ptr::write` already explicitly avoided using the wrapper, while `ptr::read` just called the wrapper (and was the reason for us observing any behavior change from rust-lang#86003 in Rust-GPU). <hr/> The codegen test I've added fails without the change to `core::ptr::read` like this (ignore the `v0` mangling, I was using a worktree with it turned on by default, for this): ```llvm 13: ; core::intrinsics::copy_nonoverlapping::<u8> 14: ; Function Attrs: inlinehint nonlazybind uwtable 15: define internal void `@_RINvNtCscK5tvALCJol_4core10intrinsics19copy_nonoverlappinghECsaS4X3EinRE8_25mem_replace_direct_memcpy(i8*` %src, i8* %dst, i64 %count) unnamed_addr #0 { 16: start: 17: %0 = mul i64 %count, 1 18: call void `@llvm.memcpy.p0i8.p0i8.i64(i8*` align 1 %dst, i8* align 1 %src, i64 %0, i1 false) not:17 !~~~~~~~~~~~~~~~~~~~~~ error: no match expected 19: ret void 20: } ``` With the `core::ptr::read` change, `core::intrinsics::copy_nonoverlapping` doesn't get instantiated and the test passes. <hr/> r? `@m-ou-se` cc `@nagisa` (codegen test) `@oli-obk` / `@RalfJung` (miri diagnostic changes)

JohnCSimon · 2021-08-22T05:34:14Z

Triage: this still has merge conflicts

eddyb · 2021-10-09T11:10:58Z

Rebased, but switched to draft because the feature-gating part (#86699 (comment)) isn't done yet.

For the record, the regression this was meant to solve, has been worked around by #87827.

bors · 2021-12-05T06:55:55Z

☔ The latest upstream changes (presumably #91475) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2022-11-27T02:24:42Z

@eddyb
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rust-highfive assigned matthewjasper Jun 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2021

eddyb added the I-nominated label Jul 1, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 1, 2021

rustbot removed the I-nominated label Jul 8, 2021

nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels Jul 9, 2021

eddyb force-pushed the reify-intrinsics branch 2 times, most recently from 1ebdd02 to 2e00f3d Compare July 15, 2021 09:56

rustbot removed the I-nominated label Jul 15, 2021

eddyb added I-nominated and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 15, 2021

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 20, 2021

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 30, 2021

rfcbot added the to-announce Announce this issue on triage meeting label Jul 30, 2021

rustbot removed the to-announce Announce this issue on triage meeting label Aug 5, 2021

eddyb mentioned this pull request Aug 6, 2021

Avoid using the copy_nonoverlapping wrapper through mem::replace. #87827

Merged

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2021

eddyb added 2 commits October 9, 2021 14:05

Allow reifying intrinsics to fn pointers.

c82cf47

rustc_mir: run the lower_intrinsics pass on MIR shims.

fddc321

eddyb force-pushed the reify-intrinsics branch from 2e00f3d to fddc321 Compare October 9, 2021 11:09

eddyb marked this pull request as draft October 9, 2021 11:09

eddyb mentioned this pull request Nov 22, 2021

core::mem::swap used to work, then broke (and later accidentally got unbroken again). EmbarkStudios/rust-gpu#804

Open

mati865 mentioned this pull request Jan 14, 2022

[mir-opt] Optimize calls to CopyNonOverlapping #83785

Closed

Mark-Simulacrum 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 Jul 24, 2022

JohnCSimon closed this Nov 27, 2022

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 27, 2022

RalfJung mentioned this pull request Nov 27, 2022

directly expose copy and copy_nonoverlapping intrinsics #81238

Merged

GrigorenkoPV mentioned this pull request Jun 17, 2024

Allow reifying intrinsics to fn pointers. (rebase of #86699) #126595

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow reifying intrinsics to `fn` pointers. #86699

Allow reifying intrinsics to `fn` pointers. #86699

eddyb commented Jun 28, 2021

rust-highfive commented Jun 28, 2021

nagisa commented Jun 28, 2021

tmiasko commented Jun 29, 2021

eddyb commented Jun 29, 2021

apiraino commented Jul 8, 2021

nikomatsakis commented Jul 9, 2021 •

edited

Loading

eddyb commented Jul 9, 2021

bjorn3 commented Jul 9, 2021

apiraino commented Jul 15, 2021

eddyb commented Jul 15, 2021

eddyb commented Jul 20, 2021

bjorn3 commented Jul 20, 2021

eddyb commented Jul 20, 2021

bjorn3 commented Jul 20, 2021

joshtriplett commented Jul 20, 2021

rfcbot commented Jul 20, 2021 •

edited by cramertj

Loading

rfcbot commented Jul 30, 2021

bors commented Aug 5, 2021

apiraino commented Aug 5, 2021

JohnCSimon commented Aug 22, 2021

eddyb commented Oct 9, 2021

bors commented Dec 5, 2021

JohnCSimon commented Nov 27, 2022

Allow reifying intrinsics to fn pointers. #86699

Allow reifying intrinsics to fn pointers. #86699

Conversation

eddyb commented Jun 28, 2021

rust-highfive commented Jun 28, 2021

nagisa commented Jun 28, 2021

tmiasko commented Jun 29, 2021

eddyb commented Jun 29, 2021

apiraino commented Jul 8, 2021

nikomatsakis commented Jul 9, 2021 • edited Loading

eddyb commented Jul 9, 2021

bjorn3 commented Jul 9, 2021

apiraino commented Jul 15, 2021

eddyb commented Jul 15, 2021

eddyb commented Jul 20, 2021

bjorn3 commented Jul 20, 2021

eddyb commented Jul 20, 2021

bjorn3 commented Jul 20, 2021

joshtriplett commented Jul 20, 2021

rfcbot commented Jul 20, 2021 • edited by cramertj Loading

rfcbot commented Jul 30, 2021

bors commented Aug 5, 2021

apiraino commented Aug 5, 2021

JohnCSimon commented Aug 22, 2021

eddyb commented Oct 9, 2021

bors commented Dec 5, 2021

JohnCSimon commented Nov 27, 2022

Allow reifying intrinsics to `fn` pointers. #86699

Allow reifying intrinsics to `fn` pointers. #86699

nikomatsakis commented Jul 9, 2021 •

edited

Loading

rfcbot commented Jul 20, 2021 •

edited by cramertj

Loading