Skip to content

Conversation

@eduardosm
Copy link
Contributor

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.

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`.
@rustbot
Copy link
Collaborator

rustbot commented Dec 26, 2025

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Comment on lines -54 to -55
// SAFETY: we can use simple float equality because when this should only be used in const
// context where Intel peculiarities don't appear
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor

@sayantn sayantn left a 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.

@eduardosm
Copy link
Contributor Author

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 target_feature no longer requires it, we can make all test functions safe fns and use instead unsafe blocks where needed. And the final goal would be to make #[simd_test] reject unsafe fns (like #[test] does).

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 unsafe keyword) and handle the rest in a follow up PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants