-
Notifications
You must be signed in to change notification settings - Fork 309
x86: remove "unsafe" from tests that do not need it #1981
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
Some test utility functions (e.g., `assert_eq_m128`) have been refactored so they are not unsafe. Many test functions (`test_mm_*`) do not call any unsafe function (besides needing target features) anymore, so they do not need to be `unsafe fn`.
| // SAFETY: we can use simple float equality because when this should only be used in const | ||
| // context where Intel peculiarities don't appear |
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 think Intel peculiarities matter much 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.
it kinda does, the docs of const_eval_select say that
Therefore, when using this intrinsic anywhere that can be reached from stable, it is crucial that the end-to-end behavior of the stable const fn is the same for both modes of execution.
Fortunately, as it turns out, EQ_OQ mode (true only if a == b, with no #IA signalling on QNaN) behaves exactly the same as a == b (= a.partial_cmp(&b) == Some(Equal)), except for some MXCSR flags which I am not too worried about. And Intel uses this exact mode for the cmpeq intrinsics.
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.
That what I was hoping for, avoiding const_eval_select and using directly the assert_eq_const macro (which uses const_eval_select to produce a simpler panic message in const-eval).
sayantn
left a comment
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.
you did leave some of the unsafe annotations (mostly) on functions that do load/store. Although the intrinsics themselves are unsafe because they take a raw pointer argument, we can still make the tests safe.
Yes, indeed, since However, since this PR is already as large as it is, I wanted first do the trivial cases (in most cases, it is just removing the |
Some test utility functions (e.g.,
assert_eq_m128) have been refactored so they are not unsafe. Many test functions (test_mm_*) do not call any unsafe function (besides needing target features) anymore, so they do not need to beunsafe fn.