-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Remove vfp2->d32 implication and fix redundant in target specs #149512
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
base: main
Are you sure you want to change the base?
Conversation
|
These commits modify compiler targets. |
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
|
r? compiler Rerolling my boss |
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.
There's also compiler/rustc_target/src/spec/targets/armv7a_nuttx_eabihf.rs which doesn't have a comment like this but maybe should be consistent in terms of target features?
Cc @no1wudi
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.
And then there are thumbv7em_none_eabihf and thumbv7em_nuttx_eabihf which currently use the vfp4d16sp target feature which doesn't even exist as a Rust feature... no idea what to do with those, should that just become vfp4 since d16 is the default? Does the sp part mean there's something extra special going on 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.
vfp4d16sp in thumbv7em_nuttx_eabihf is derived from thumbv7em_none_eabihf , I guess this is from clang?
|
r? @RalfJung |
Fixes #149399 - this implication was likely a mistake and isn't enforced by LLVM. This is is a breaking change and any users specifying
vfp2/3/4via-Ctarget-featuresor thetarget_featureattribute will need to addd32in to get the same behaviour. The target features are unstable so this is ok for Rust, and this is necessary as otherwise there's no way to specify avfp2-d16configuration, for example.I expect these targets would have been broken by #149173 as
-d32would have disabled any+vfpXfeature before it. With the removal of the implication the-d32went back to being unnecessary, but I've removed it anyway.As @RalfJung pointed out, thumbv7a-nuttx-eabihf looks to have been relying on this implication so I've added
+d32to it's target spec.-neonmight be unnecessary too in many of these cases, but some default CPUs that Rust specifies will turn Neon on so that needs a bit more care. I can't see any LLVM cpus that enable D32.