Tracking Issue for #[instruction_set] attribute (RFC 2867)

[nikomatsakis]

This is a tracking issue for the RFC "#[instruction_set(...)] attribute" (rust-lang/rfcs#2867).

The feature gate for the issue is #![feature(isa_attribute)].

The lang-team liaison for this feature is @pnkfelix.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

• Implement the RFC (Implementation of RFC2867 #76260)
• Adjust documentation (Document RFC 2867: instruction_set attribute reference#1253)
• Stabilization PR (see instructions on rustc-dev-guide)

Unresolved Questions

• How do we ensure that instruction_set and inline assembly always interact correctly? This isn't an implementation blocker but needs to be resolved before Stabilization of the attribute. (Currently, LLVM will not inline a32 functions into t32 functions and vice versa, because they count as different code targets. However, this is not necessarily a guarantee from LLVM, it could just be the current implementation, so more investigation is needed.)

Implementation history

[xd009642]

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 CodegenFnAttrs in librustc_middle. From there I imagine it's just a case of getting it to the llvm codegen. I'll likely open a WIP PR and ask any questions I have there once I've made some progress

[xd009642]

@pnkfelix do you mind if I message you for guidance once I have a more concrete idea of what my questions are?

[jonas-schievink]

I'll write up some pointers.

• The symbols to be used in the attribute syntax have to be added to symbol.rs.
• The instruction_set attribute has to be added as a gated attribute in builtin_attrs.rs
• A new instruction_set field needs to be added to CodegenFnAttrs, and a new InstructionSet enum needs to be added.
• The codegen_fn_attrs query has to look at the attributes and compute the right value for CodegenFnAttrs. It also has to check:
  • That the instruction_set feature flag is enabled. (actually, adding the attribute might already do this)
  • That the current compilation target (available in tcx.sess.target.target) supports switching between ARM and Thumb mode (notably, this is the case for the thumbv4t-... target, but not for the thumbv6/7/8m-... targets; I'm not sure what the best way to codify this is, maybe by extending target specs).
  • That the attribute is syntactically correct.
  • ...and otherwise, emit an appropriate error (that should also be tested for).
• The LLVM codegen backend has to be adjusted to apply the target feature attribute to the function, this happens in attributes::from_fn_attrs.
  Thumb mode is enabled on ARM targets by enabling the thumb-mode target feature. If thumb mode is to be disabled, the target feature has to be turned off by prefixing it with - instead of +. This can be probably be done here.

[xd009642]

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 codegen_fn_attrs and I guess talk to @Lokathor to see what thumb targets I need to do this for.

Thanks that fills in a lot of my open questions 😄

[Lokathor]

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 arm are ARM with a32 as the default codegen, and then all the targets in tier2 and tier3 that start with thumb are ARM with t32 as the default codegen. For example, thumbv4t-none-eabi, thumbv6m-none-eabi, and so on.

Ideally this works for any target_arch="arm" target, even a custom target profile.

[ketsuban]

If there's already a way to tell a target that starts with arm from one that starts with thumb (which tells you whether the default is -thumb_mode or +thumb_mode) I think I agree with @jonas-schievink that the best way forward is probably a new boolean field (has_thumb_interworking?) in the target spec.

[xd009642]

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 .

[joshtriplett]

Current status from today's @rust-lang/lang meeting: updating to account for targets that only support one or another instruction set.

[xd009642]

@joshtriplett are there minutes for the meeting? Or should I just wait till the RFC or issue is updated and adjust the PR?

[Lokathor]

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

• Your PR is waiting for some T-compiler feedback about how to handle targets that can't switch their instruction set.
• That probably the solution would be to add an extra flag about if the target allows instruction set switching.

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.

[xd009642]

So far there's an additional has_thumb_interworking option on the targets, and only ones with that as true can have the instruction_set applied otherwise it's an unsupported target (false being the default). But yeah I need some CI feedback and potentially some feedback on the UI tests

[Lokathor]

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.

[lifning]

Is it possible for core library functions to get inlined despite not being specified as one ISA or the other?
My use case is wanting to possibly take advantage of instructions like a32's ldmia/stmia for some heavy lifting in a perf-critical bit of interrupt handler code living in IWRAM, but looking at rustc's emitted asm, i notice usages of things like core::array::<impl core::ops::index::Index<I> for [T; N]>::index still require branching into thumb_func's that live in ROM.

[ketsuban]

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 build-std attribute to be aware of has_thumb_interworking and include a copy of the standard library compiled for the other code target.

[lifning]

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

[Lokathor]

Update 2021-04-06

Picking 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: https://rust.godbolt.org/z/9v914d8fe https://rust.godbolt.org/z/91cPYq8bn. Note the build args.

• -Copt-level=3: this is as release as it gets
• --target armv5te-unknown-linux-gnueabi: all code (including core) is a32 by default, and t32 can be used.

The exact details of the ARM calling conventions aren't quite important here, but the thing to note within the assembly output is:

• vol_read_a32 gets inlined. It uses the read_volatile method which is also inlined. You get a fully zero-cost ldr r4, [r0].
• vol_read_t32 does not get inlined. It uses the read_volatile method which doesn't get inlined, you get a shockingly expensive double function call for what should be a single instruction.

Meanwhile let's look at if we can paper over the issue with inline assembly:

• asm_read_a32 is inlined, but now it's two instructions instead of one. more on that in a moment.
• asm_read_t32 is not inlined but (because it's not calling to an intrinisc) at least it's only a single function call and not a double function call.
• A moment ago I said there's an extra move instruction. Depending on how you jigger the assembly you can get that move extra move either in the asm_read_a32 or the asm_read_t32. Assuming that you use the same assembly pattern for both functions, whichever pattern you pick you have to slow one of them down by a single move. That's not zero cost.

"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:

• core::iter::traits::iterator::Iterator::copied: does literally nothing at all.
• core::slice::<impl [T]>::iter: a single add operation.
• core::convert::num::<impl core::convert::From<u8> for u32>::from: this does a useless input&255 op as a way to turn u8 to u32 for reasons I can't even guess at. Note that if we remove the instruction_set attribute from this function the properly inlined version doesn't have the 255 anywhere in the code, so somehow llvm is just tripping itself over nothing at all.

Summary

• Does It Work? Yes
• Is It Zero Cost? Not even remotely
• Is It Fit To Be Stable? Interesting question I'm so glad you asked. I think arguably yes. Like the one situation where people are using this (folks doing GBA stuff) is already stuck on Nightly until either the thumbv4t-none-eabi target becomes Tier 2 so that we don't have to use build-std, or build-std becomes stable. So, making this stable immediately isn't going to suddenly unblock Stable GBA developments. However... I don't think that there's a way to improve the developer interface for this attribute. The interface between the programmer and the compiler of "this is how you declare this quality for the function you're describing to the compiler" is so simple that there's not really any other way that it could work. The performance problems are "just" a codegen quality issue. That's a very load-bearing "just", but it's true. The problem is that rust relies a crazy amount on inlining, more than you ever normally think about, and so for the codegen here to get better we need to make inlining happen properly for functions using the non-default instruction set. Maybe more stuff gets inlined in MIR, maybe rustc looks at the call-graph of alternate instruction set functions and then for each thing it calls that doesn't have inline assembly and doesn't have a specific instruciton_set assigned it just emits LLVM IR for the function twice, once in a32 and once in t32, and then LLVM can do its thing at the cost of a little compile time. Whatever the fix ends up being, I don't think it would change the current developer interface for using the attribute in code.
• Overall Current Status: It was mentioned in a lang team meeting that some things can be rated as +0 instead of +1, and right now this is a good example of a +0 unstable feature.

[repnop]

one thing for consideration is having this not be its own stand-alone attribute applied to functions, but being an available option as a #[repr(instruction_set = "...")] instead since its affecting the representation of the function. food for thought :)

[nikomatsakis]

@Lokathor any update this month? Thanks for the update last month, we have a meeting next Wed and we'll talk it over.

[Lokathor]

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.

[Lokathor]

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

[joshtriplett]

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?

[joshtriplett]

ping @pnkfelix

[pnkfelix]

Okay, I reviewed the update from last year and fixed the first godbolt link (asm! has stabilized since it had been posted).

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

[rfcbot]

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

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

Concerns:

• documentation-pr resolved by Tracking Issue for #[instruction_set] attribute (RFC 2867) #74727 (comment)

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.

[xd009642]

First stabilisation report so let me know if I've done anything wrong 🙏

Request for Stabilization

Summary

The 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:

#[instruction_set(arm::a32)]
fn arm_code_fn() {
    // function compiled using arm code
}

Thumb code:

#[instruction_set(arm::t32)]
fn thumb_code_fn() {
    // function compiled using thumb code
}

Documentation

Currently, 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.

Tests

• https://github.com/rust-lang/rust/blob/master/src/test/ui/error-codes/E0778.rs check error reporting for missing instruction set
• https://github.com/rust-lang/rust/blob/master/src/test/ui/error-codes/E0779.rs check error reporting for invalid instruction set

Unresolved Questions

• Code generation is poor, what work can improve this in future is unresolved (to my knowledge)

[nikomatsakis]

@rfcbot concern documentation-pr

We need an open PR adding this to the Rust reference!

[xd009642]

@nikomatsakis PR opened with an initial go at the docs: rust-lang/reference#1253

[nikomatsakis]

@rfcbot resolve documentation-pr

Thanks @xd009642 !

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

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.

This will be merged soon.