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

[nikomatsakis]

This is a tracking issue for the RFC "isa-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

• [x] Implement the RFC (https://github.com/rust-lang/rust/pull/76260)
• [ ] Adjust documentation (see instructions on rustc-dev-guide)
• [ ] 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 :smile:

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