-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Arm bare-metal target doc changes #124404
Conversation
Now explains how to, for example, support a Cortex-M55 with FPU and Integer Helium.
Some changes occurred in src/doc/rustc/src/platform-support cc @Nilstrieb |
I do not know enough about ARM to comment whether the content is correct, but you do as a target maintainer so I trust you there. @bors r+ rollup |
This comment has been minimized.
This comment has been minimized.
@bors r- nevermind |
What went wrong? |
linkcheck #124404 (comment) |
Yeah I saw that and I don't know what I did wrong. I added src/doc/rustc/src/platform-support/thumbv6m-none-eabi.md, and I linked to it. Do I need to tell it about the new file? Or is there a typo I just can't see? |
New files need to be added to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few minor notes. At a higher level though, would it be best to consolidate the hf
and non-hf
documents for the variants that have both? There's a lot of duplicated information that would have to be manually kept in sync as-is.
I've done this in the most recent commit. If we don't like it, I can drop it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arm consistently uses title case for their own name and architecture names ("Armv6-M", "Armv7E-M", etc.) in their docs these days, so probably should do that here. Right now it's a mix.
Oof. OK, I've done it. But I'm not sure I like how Arm spell it. Also, I didn't touch "ARM64" (which is a Microsoft term, not an Arm term). |
## Common Target Details | ||
|
||
This documentation covers details that apply to a range of bare-metal targets | ||
for 32-bit ARM CPUs. In addition, target specific details may be covered in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arm
Looks great! I will try and assemble similar tables for the Cortex-R's in the next week or so. |
Yes it looks weird, but this is how Arm write it now. I left ARM64 alone, because it's a Microsoft/Apple term but not an Arm term (they have Armv8-A and Armv9-A architectures, which say that A64 instructions are executed when in the Aarch64 state), and I don't want to get into that, especially for a Tier 1 target.
d92aa43
to
fcaba9c
Compare
Ok one more update done - I added a section clarifying the terms Arm ISA, Thumb ISA, Thumb-2 ISA, A32 and T32. |
Thanks! @bors r=chrisnc rollup |
☀️ Test successful - checks-actions |
Finished benchmarking commit (cfb2410): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 676.596s -> 674.001s (-0.38%) |
| Any | No | Yes | None | None | | ||
| Cortex-M4 | No | Yes | `cortex-m4` | `+soft-float` | | ||
| Cortex-M4F | SP | Yes | `cortex-m4` | None | | ||
| Cortex-M7 | No | Yes | `cortex-m7` | `+soft-float` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this is a problematic piece of advice to give, since this target feature is also implicated in the soundness issue tracked at #116344.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although as we discussed extensively on Zulip currently we know of no other way of stopping target-cpu=cortex-m7
(or similar) from emitting FPU instructions like vadd.f32
, yet it's reasonable to want the Cortex-M7 specific instruction scheduling optimisations even if your Cortex-M7 doesn't have an FPU.
This feature is totally unsound on *-unknown-none-eabihf
, but it's both sound and useful on *-unknown-none-eabi
. The 'target-feature' checker can currently only look at the target-architecture, and not the full target triple, so it's unclear where we go from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target-feature checker is still WIP, and will be improved to handle this, somehow.
But the docs maybe should call out that this same flag is unsound to use on other targets (until rustc reaches the point where it is able to tell users about this directly).
Also, it is rather unusual for stable docs to recommend unstable/unsupported features. Using these flags will cause warnings on current compilers! (And the only reason those warnings are not hard errors is that target features are a terrible mess and we are grandfathering in all LLVM features, and hoping that doesn't break too many things.) |
We have an issue for this now: #130988 |
Updates the Arm bare-metal target docs:
eabi
andeabihf