Tracking Issue for `#[instruction_set]` attribute (RFC 2867) · Issue #74727 · ru...
source link: https://github.com/rust-lang/rust/issues/74727
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Tracking Issue for #[instruction_set]
attribute (RFC 2867)
#74727
1 of 3 tasks
nikomatsakis opened this issue on Jul 24, 2020 · 31 comments
Comments
This is a tracking issue for the RFC " The feature gate for the issue is The lang-team liaison for this feature is @pnkfelix. About tracking issuesTracking issues are used to record the overall progress of implementation. StepsUnresolved Questions
Implementation history |
added B-RFC-approved Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature.
labels
added the F-isa_attribute Related to #[instruction_set] attribute introduced by RFC 2867 label
Contributor
xd009642 commented on Aug 22, 2020
I'm going to take a shot at implementing this RFC. I've already got an initial idea of where to start, adding it to |
Contributor
xd009642 commented on Sep 1, 2020
@pnkfelix do you mind if I message you for guidance once I have a more concrete idea of what my questions are? |
I'll write up some pointers.
|
So I've added the symbols to symbol.rs, and instruction_set to CodegenFnAttrs added an InstructionSet enum, updated codegen_fn_attrs (although not all of the sub-bullet points, it's very happy path only right now). I'll look at the target stuff next since that seems to be the biggest part I'm missing in Thanks that fills in a lot of my open questions |
Contributor
Lokathor commented on Sep 3, 2020
I don't know the best way to, from inside the compiler, tell if the target is an ARM arch target. And, even then, to tell if the default codegen will be a32 or t32. I can say that, looking at the platform support page, all the tier2 and tier3 targets that start with Ideally this works for any |
Contributor
ketsuban commented on Sep 3, 2020
If there's already a way to tell a target that starts with |
Contributor
xd009642 commented on Sep 6, 2020
So I've attempted to do all the points laid out above to the best of my effort (minus the tests for appropriate errors as I had a feeling my errors are likely to change). There's probably a lot to change as this is my first time working on the compiler, but I think it's ready for some initial feedback - #76260 . |
Current status from today's @rust-lang/lang meeting: updating to account for targets that only support one or another instruction set. |
Contributor
xd009642 commented on Sep 28, 2020
@joshtriplett are there minutes for the meeting? Or should I just wait till the RFC or issue is updated and adjust the PR? |
Contributor
Lokathor commented on Sep 28, 2020
@xd009642 I was at the meeting today and attempted to convey the status, though I may have done so inexpertly. What I said was that:
I don't think that the RFC needs an update at all, or that your PR needs any changes other than whatever T-compiler asks for. Feel free to work out any blockers/concerns on the PR with them, and also feel free to comment anything more if I missed a detail. |
Contributor
xd009642 commented on Sep 28, 2020
So far there's an additional |
Contributor
Lokathor commented on Oct 9, 2020
Update: PR merged, starting tomorrow you can use it in your nightly work, presumably we'll have a trial period of at least few releases before considering possible stabilization. |
Is it possible for core library functions to get inlined despite not being specified as one ISA or the other? |
Contributor
ketsuban commented on Nov 25, 2020
The problem as I understand it is there's only one (T32) copy of the standard library and Rust just emits calls into it despite the function being A32, which LLVM then refuses to inline. A solution could be for Cargo's unstable |
lifning commented on Nov 25, 2020 •
There're definitely other things besides core that the lack of ability to inline kind of makes a drag too, such as the phantom_fields defined by https://docs.rs/crate/gba-proc-macro (which i think my current best bet is to just.. duplicate each function into foo() and a32_foo() versions and call the latter in my code, but I don't like the idea of forking every dependency crate and making duplicate a32 versions of every stub function i need to call) For this case it would definitely benefit me to be able to compile arbitrary dependencies in both modes too, though I'm not sure what that would look like ideally. A lot of code here isn't currently delivering on the zero-cost abstraction promise, in other words :P |
Update 2021-04-06Picking a target that supports using both a32 and t32 code, and that is tier2 so we can see it on goldbolt, please look at the following code:
The exact details of the ARM calling conventions aren't quite important here, but the thing to note within the assembly output is:
Meanwhile let's look at if we can paper over the issue with inline assembly:
"Loka that's just one example though?" Well, sure, but it's a very important one because being able to use these zero-cost intrinsics is the sales pitch of rust. Here's another example: https://rust.godbolt.org/z/bG3bhTd4z:
Summary
|
Contributor
repnop commented on Apr 7, 2021
one thing for consideration is having this not be its own stand-alone attribute applied to functions, but being an available option as a |
Contributor
Author
nikomatsakis commented on May 13, 2021
@Lokathor any update this month? Thanks for the update last month, we have a meeting next Wed and we'll talk it over. |
Contributor
Lokathor commented on May 13, 2021
I did have a build that managed to create an illegal instruction in debug mode but not release mode, though that might be related to the incremental compilation bug that was recently resolved. I haven't had time to check again since that announcement. |
Contributor
Author
nikomatsakis commented on Jul 7, 2021
Hey @Lokathor -- so we discussed this in our planning meeting today. Re-reading the April update you posted, it seems like this may indeed be ready for stabilization. Do you know whether anyone is using it in practice? In the blog post I publish today I can put out a "call for testing", as well. |
Contributor
Lokathor commented on Jul 7, 2021
I've only heard from the five or so people working on the |
@nikomatsakis Having worked with this some more, I can say that things are ready for stabilization. the code that you get out of this is pretty bad, but that's just a quality-of-life issue. The interface for how a user marks functions with this attribute wouldn't change at all even if the compiler gets smarter about codegen. EDIT: particularly, this works with inline_asm, which was the major concern way back when. |
added the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label
Member
joshtriplett commented on Jun 21
We discussed this in today's @rust-lang/lang meeting. Kicking off stabilization of this seems fine. @pnkfelix, as the liaison, want to start a stabilization FCP if you concur? |
added the S-tracking-ready-to-stabilize This is ready to stabilize; it may need a stabilization report and a PR label
Okay, I reviewed the update from last year and fixed the first godbolt link ( Its an interesting question, whether to stabilize something that has clear performance issues (in terms of not meeting typical zero-cost expectations). But that is a quality-of-life issue; if users can find utility in this feature even in its current state, then that's an argument for stabilization. I'll go ahead and fire off a stabilization FCP. @rfcbot fcp merge |
Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
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. |
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
First stabilisation report so let me know if I've done anything wrong Request for StabilizationSummaryThe instruction set attribute provides a new function attribute for architectures which support more than one form of machine code. Currently, this is just implemented for CPUs in the ARM family which support a program switching between "ARM code" and "Thumb code". Although other instruction sets could be added in future as unstable features. These two instruction sets appear as follows: Arm code:
Thumb code:
DocumentationCurrently, the only usage is homebrew GameBoy Advance development for ARMv4t and the RFC. Given the number of uses of this feature is currently limited to these people this is potentially fine. TestsUnresolved Questions
|
removed the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label
Contributor
Author
nikomatsakis commented 23 days ago
@rfcbot concern documentation-pr We need an open PR adding this to the Rust reference! |
Contributor
xd009642 commented 23 days ago
Contributor
Author
nikomatsakis commented 9 days ago
added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label
rfcbot commented 8 days ago
This is now entering its final comment period, as per the review above. |
removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
No one assigned
None yet
No milestone
No branches or pull requests
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK