-
Notifications
You must be signed in to change notification settings - Fork 311
remove fp16 target feature from some vreinterpret intrinsics #1991
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
Conversation
|
Looks good to me, thank you! |
| assert_instr(nop) | ||
| )] | ||
| #[target_feature(enable = "neon,fp16")] | ||
| #[cfg_attr(target_arch = "arm", target_feature(enable = "fp16"))] |
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.
I don't believe the fp16 target feature is required on 32-bit either.
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.
This is what was done in #1978, for stores and loads.
@adamgemmell, do you have ideas?
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.
vreinterprets are NOPs, so there's no issues with the instruction not being available on 32 bit platforms. If it builds and the tests pass then it's fine to remove for both sets of targets
aa46f8e to
1b44fd0
Compare
|
The second commit is for passing CI. rust-lang/rust#151529 |
|
For context: this includes a fix for miri failing to build |
Usage of them is accepted by clang without
fullfp16(https://godbolt.org/z/dhffa4YT9). It seems that they are missing in #1978.cc @adamgemmell, in case I misunderstood