support hardware sqrt for f32 on arm32 targets - #1300
Conversation
|
seems like I murdered CI |
This comment has been minimized.
This comment has been minimized.
a9459c8 to
020bec7
Compare
|
Seems like the only failure is on the libm MSRV check due to the use of |
|
The ABI thing can be worked around with the build script: update compiler-builtins/libm/configure.rs Lines 99 to 126 in b2a59c9 target_abi_eabihf and target_abi_eabi, after picking up CARGO_CFG_TARGET_ABI.
It looks like you could also check for the Is |
|
Hadn't even considered utilizing the build script, that might also make it easy to implement f64 The output is identical for thumbv7em-none-eabihf and both non-hf variants. |
|
Is (Please confirm what |
|
One other note is that you may want to consider doing something similar for |
Hmm. It's odd that fpregs,vfp2,vfp3,d32 are enabled for soft-float targets by default. Given that they are enabled for armv7-unknown-linux-gnueabi and armv7a-none-eabi but not for arm-unknown-linux-gnueabi, armv7r-none-eabi, and thumbv7em-none-eabi, I guess LLVM infer that fregs are available for |
|
Spent some time looking into this yesterday and it's a weird situation. The The whole vfp* family is a chain of features each of which is a superset of those beneath it, and which are most often implied rather than being directly enabled. Thumbv8m uses You can see that something like Armv7a has the Doing a feature test for I can finish up reworking the sqrtf feature gating, but I think a proper f64 sqrt implementation should probably be it's own PR. Might even be best to block it on the stabilization of arm features. Targets which have f64 tend to be more powerful than those with only f32 (f32 only are mostly embedded targets), so it could be argued that they benefit less from the hardware acceleration and can hold off just a bit longer with the current software implementation. |
My understanding is that the eabihf suffix only impacts the use of FP registers for passing values per this explanation. It seems to be for literally nothing more than to constrain the ABI. You can still use fp ops within your code and if you have features enabled that let you use hardware fp ops then LLVM should still emit them. The Cortex-A7 lists |
My understanding is that
So gating the module on
The problem is that I assume that will be clarified as part of @adamgemmell's refactor (mentioned at rust-lang/rust#161826) but for the time being, I'd prefer to gate the individual function on
Up to you! I'm happy to take both together since they're pretty similar, with a |
|
If the only safe option is to use flags only available on nightly then it's probably best to delay this until |
|
I think that's orthogonal, if you use Even once |
|
|
|
Seems like |
a8e9ab8 to
0c38dfb
Compare
This comment has been minimized.
This comment has been minimized.
0c38dfb to
14f60ee
Compare
|
Remaining failures are due to ubuntu-26.04 benches using |
7bca3b5 to
b697783
Compare
Ah yeah I guess it's only old nightlies that don't work. Not the biggest deal as long as we don't break stable, but should be an easy fix once we get the syntax right. |
Before this is merged I'd like to again try to make the case for treating I'd argue that users explicitly opting to use the hard float ABI, then using unstable features to disable floating point operations just don't exist. Even though it's theoretically possible, it's a contradicting combination that's entirely impractical. And I can't foresee a world where a new target is added which somehow manages to reasonably combine these two opposing behaviors. |
|
My concern is still that the feature will be stable in the near future (hopefully!), and https://github.com/rust-lang/rust/blob/70222712809cd5cc1718ed8995914a1cbacb6b92/compiler/rustc_target/src/target_features.rs#L181 does not say We can keep discussing because maybe there's an alternative (detect whether or not |
I don't think that My argument is that even though the If you'd still rather have both gated behind the features then I totally understand, and it's probably fine to merge for now, assuming it all looks good to you. |
|
Use of the There's alternative implementations to a circular feature dependency - we could tie them together or fold them together under the same feature name, which have slightly different options available for splitting them post-stabilisation.
Reading this suggests to me that we shouldn't expose I noticed the fact that FPU features are enabled on some a32 softfloat targets too and I think that's a mistake. I wouldn't use 32 bit arm target specs as an authoritative source on how something should work 😄 |
I'm advocating for gating the "Rust targets ending in eabihf use the so-called hard-float ABI: functions which take f32 or f64 as arguments will have them passed via FPU registers. These targets therefore require the availability of an FPU and will assume some baseline level of floating-point support is available (which can vary depending on the target)."
What's currently being proposed wouldn't require direct access to
Even though it's weird this may very well be intentional, from the same rustc platform support doc linked above: "Rust targets ending with eabi use the so-called soft-float ABI: functions which take f32 or f64 as arguments will have those values packed into integer registers. This means that an FPU is not required from an ABI perspective, but within a function floating-point instructions may still be used if the code is compiled with a target-cpu or target-feature option that enables FPU support." |
|
Still agree with Trevor that
Interesting, thanks for the link |
|
I also agree that |
|
@adamgemmell how close do you think
Yeah, I'd prefer to get this merged and we can figure something out in a followup. Just needs the two open things resolved to take care of warnings. I guess one option is to have a list of current eabihf targets and manually set |
8f243b8 to
e3c7088
Compare
Note that target features on ARM are not yet stable (tracked by [1]) so this only actually gets used on nightly. [1]: rust-lang/rust#150246 [ add commit body - Trevor ]
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I'm scheduled to work on them next quarter, and I'd like to gather up and run the pros/cons by people before stabilising any features, in particular FPU features. Once set we've moved to stabilise the features relatively quickly in the past (a few months?). Is the motivation mainly unblocking using these functions on nightly? |
|
Oh right sorry, you did say that on the issue. For these specific features, having them stable would mean mean we get to use these and future asm ops without nightly—I was just weighing whether a workaround is worth it for now. Or, if other features are blocked on naming for the time being, whether there might be a subset we could stabilize sooner. |
|
The combinatorial explosion of fpu features might be a contention point, for example |
|
@tgross35 @adamgemmell Sorry to bring this back from the dead. Was researching the required feature levels for ARM32/Thumb fma and RISC-V sqrt/fma and found that version 1.97 stabilized CfgStableToggleUnstable, which is specifically for unstable features which can be used in Is this something worth pursuing for the ARM features, ahead of full stabilization of |
The pull to stabilize d, e, and f was actually reverted, but only because of concerns around the |
|
I think that, based on what Adam was saying, there's a chance that the names might not actually stay |
Following up on this discussion.
Currently this uses a bit of a janky trick to work around a rustc bug. Since we can't use sreg to operate on input
xin-place, we instead cast it to an integer, load it into an integer register, then move it to a floating point register where we can use sqrt. A little annoying, but 3 instructions is still far better than a whole software sqrt routine lol. Hopefully the fix for the bug will land in the next couple of weeks and will be in stable 1.100.0. The original bug report can be found here.I tried to mimic the structure of previous implementations, but let me know if anything should be changed. Was also going to do the f64
sqrtvariant for chips like the M7, M55, M85, etc., but I'm not sure there's currently a reliable (and stable) way to detect both HF ABI and fp64 support.