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

Revise arm platform notes regarding soft float #130987

Merged
merged 5 commits into from
Oct 21, 2024

Conversation

thejpster
Copy link
Contributor

This PR updates the Arm microcontroller platform docs to recommend -fpregs instead of +soft-float as discussed on Zulip

These arrived in rust-lang#125690, I think by mistake.
@rustbot
Copy link
Collaborator

rustbot commented Sep 28, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 28, 2024

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

@thejpster thejpster changed the title Revise arm platform notes soft float Revise arm platform notes regarding soft float Sep 28, 2024
Has the same effect, but turning off a feature matches the other flags better than turning on a feature (which actually turns off a feature).
@thejpster thejpster force-pushed the revise-arm-platform-notes-soft-float branch from b6a713b to 1bec022 Compare September 28, 2024 17:20
@ehuss
Copy link
Contributor

ehuss commented Sep 30, 2024

@rustbot ping arm

Can an Arm maintainer review this, please?

Or, cc @RalfJung, since I've noticed the two of you discussing this on Zulip, if you want to review this.

@rustbot rustbot added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Sep 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2024

Hey ARM Group! This bug has been identified as a good "ARM candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @adamgemmell @davidtwco @hug-dev @jacobbramley @JamieCunliffe @joaopaulocarreiro @raw-bin @Stammark

@thejpster
Copy link
Contributor Author

thejpster commented Oct 1, 2024

AIUI there are no Arm maintainers for M-Profile, only for the Aarch64 A-Profile. Although they offered to help on a best-effort basis.

The designated maintainer is the Cortex-M Team in the Embedded Devices Working Group. I don't think you can ping them on github but I'll post this on their matrix room.

@jamesmunns
Copy link
Member

jamesmunns commented Oct 1, 2024

CC @adamgreig @thalesfragoso @newAM

Copy link
Contributor

@jacobbramley jacobbramley left a comment

Choose a reason for hiding this comment

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

This seems to do the right thing (at least for the cases I tried and based on the Zulip discussion), and no alarm bells went off when I discussed it with our LLVM teams.

One caveat we noticed — which is really a separate concern — is that there's no way to specify eabihf with no FP. That sounds nonsensical, but it's potentially useful with integer MVE, which uses the same registers. Clang achieves that with +nofp, which seems to get translated into -mfpu=none, but I can't find a way to control that through Rust.

@thejpster
Copy link
Contributor Author

Does Integer MVE let you use s0 and d0 even without an FPU?! Wild.

@jacobbramley
Copy link
Contributor

jacobbramley commented Oct 11, 2024

Does Integer MVE let you use s0 and d0 even without an FPU?! Wild.

Yes, at least for the few instructions you'd need to use this ABI. I've not done any real work on these devices so can't claim to be an expert, but the Armv8-M reference manual says that the necessary VMOV instructions are available with either FP or MVE.

The FP/vector register file is overlaid so that q0 aliases d1:d0 and s3:s2:s1:s0, just like A-class AArch32's Neon registers. You could read d0 as "the lower 64 bits of q0", which might feel more natural in a world without FP hardware. There's a layout diagram under rule RBBYH, page 180 in version B.y of the manual.

@thejpster
Copy link
Contributor Author

Does Integer MVE let you use s0 and d0 even without an FPU?! Wild.

Yes, at least for the few instructions you'd need to use this ABI. I've not done any real work on these devices so can't claim to be an expert, but the Armv8-M reference manual says that the necessary VMOV instructions are available with either FP or MVE.

The FP/vector register file is overlaid so that q0 aliases d1:d0 and s3:s2:s1:s0, just like A-class AArch32's Neon registers. You could read d0 as "the lower 64 bits of q0", which might feel more natural in a world without FP hardware. There's a layout diagram under rule RBBYH, page 180 in version B.y of the manual.

OK, I added some notes to mention that this is a potentially valid setup.

@ehuss
Copy link
Contributor

ehuss commented Oct 21, 2024

Thanks! If anyone has any objections or changes, feel free to post an update.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 21, 2024

📌 Commit 5cc1c7b has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2024
@bors
Copy link
Contributor

bors commented Oct 21, 2024

⌛ Testing commit 5cc1c7b with merge edbd939...

@bors
Copy link
Contributor

bors commented Oct 21, 2024

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing edbd939 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 21, 2024
@bors bors merged commit edbd939 into rust-lang:master Oct 21, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 21, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (edbd939): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
2.9% [2.9%, 2.9%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.9% [2.9%, 2.9%] 1

Max RSS (memory usage)

Results (primary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.3% [2.3%, 2.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.3% [2.3%, 2.3%] 1

Cycles

Results (primary 2.8%, secondary -7.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.8% [2.8%, 2.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-7.7% [-8.4%, -7.3%] 3
All ❌✅ (primary) 2.8% [2.8%, 2.8%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 781.937s -> 781.116s (-0.10%)
Artifact size: 333.66 MiB -> 333.71 MiB (0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Oct 21, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Oct 29, 2024

Noise, this only updated documentation.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants