Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a macro for defining intrinsics with the Rust ABI #127885

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

Migrating intrinsics at this time seems to come with a lot of boilerplate in the form of attributes and empty bodies (see #127337 for an example). Add a macro that simplifies this, and makes the transition from extern "rust-intrinsic" intrinsics much cleaner, and then make use of it.

Cc #93145

r? @oli-obk

Migrating intrinsics at this time seems to come with a lot of
boilerplate in the form of attributes and empty bodies (see
<rust-lang#127337> for an example). Add a
macro that simplifies this, and makes the transition from `extern
"rust-intrinsic"` intrinsics much cleaner.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 17, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Jul 17, 2024

There are a couple unfortunate things here:

  • It doesn't seem to be possible to forward $foo:item fragment specifiers to $(tt:tt)* or exact tokens, and capturing optional specific keywords isn't great (more reason for [RFC] Named macro capture groups rfcs#3649 yay), so I can't figure out any way to allow mixing fn with const fn, const unsafe fn, etc in the same macro invocation. Maybe there is a better way to do this, I need to think on it for a bit.
  • Something is weird with generics - for any function that has them, it complains about having the wrong count (e.g. expected 0 but got 1), even though the function appears to expand properly (just quickly checking via cargo expand). Any ideas here?
  • I thought some of the goal with the migration is to allow e.g. clif or gcc to not override some intrinsics and just let them panic with unimplemented!. This seems contrary to what rustc_intrinsic_must_be_overridden sounds like - what is the exact intent here? Or does this just mean that it must be overridden only iff the compiler recognizes it as an intrinsic.

This isn't 100% complete, e.g. generics with ? don't yet match, and there might be something better for repetition mixing const/unsafe/default. Just wanted to get some feedback before getting too into it.


/// Raises an `f16` to an integer power.
///
/// The stabilized version of this intrinsic is
/// [`f16::powi`](../../std/primitive.f16.html#method.powi)
#[rustc_nounwind]
pub fn powif16(a: f16, x: i32) -> f16;
pub unsafe fn powif16(_a: f16, _x: i32) -> f16;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these float intrinsics could probably just be safe if there are no preconditions, not as part of this PR though.

@tgross35
Copy link
Contributor Author

The macro could probably also be tweaked to allow specifying the fallback body

@bors
Copy link
Contributor

bors commented Jul 18, 2024

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

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2024

  • and just let them panic with unimplemented!

the point was to have a meaningful body that emulates the behaviour with potentially more expensive operations.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2024

I am not sure about this change.

I would prefer to just use item fragments and add the unreachable bodies and the must_be_overridden attribute to all intrinsics.

@tgross35
Copy link
Contributor Author

If rustc_intrinsic and rustc_intrinsic_must_be_overridden almost always go together, could they maybe just be combined in the compiler such that one implies the other?

I am not sure about this change.

I would prefer to just use item fragments and add the unreachable bodies and the must_be_overridden attribute to all intrinsics.

As in, $( $fn_sig:item );+ that matches

foo();
bar();

?

Unfortunately macro parsing is pretty insistent that the ; is part of the item, and item can't be forwarded to token trees or exact tokens so it's not possible to extract it in a separate pattern.

I might be able to improve something here, just need to fight with the macro system a bit.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2024

could they maybe just be combined in the compiler such that one implies the other?

the macro can just add the #[rustc_intrinsic] attribute and let the must_be_overridden be added at each site, together with { unreachable!() } instead of ;

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2024

r? libs I'm going on leave for 4 months

@rustbot rustbot assigned workingjubilee and unassigned oli-obk Jul 31, 2024
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something in the direction of oli's review seems correct to me. Basically "please make the macro less clever".

@rustbot rustbot 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 Aug 13, 2024
@alex-semenyuk alex-semenyuk 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2024
@workingjubilee workingjubilee added A-intrinsics Area: Intrinsics F-core_intrinsics Issue in the "core intrinsics" for internal usage only. labels Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intrinsics Area: Intrinsics F-core_intrinsics Issue in the "core intrinsics" for internal usage only. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants