Skip to content

Add missing runtime tests for SSE alias intrinsics#2041

Open
ArunTamil21 wants to merge 4 commits intorust-lang:mainfrom
ArunTamil21:add-alias-tests
Open

Add missing runtime tests for SSE alias intrinsics#2041
ArunTamil21 wants to merge 4 commits intorust-lang:mainfrom
ArunTamil21:add-alias-tests

Conversation

@ArunTamil21
Copy link
Contributor

@ArunTamil21 ArunTamil21 commented Feb 26, 2026

Add missing runtime tests for SSE alias intrinsics

While working through issue #798, I noticed several alias intrinsics had no
runtime tests even though their underlying functions were already tested.

This PR adds tests for:

  • "test_mm_cvt_ss2si" (alias for "mm_cvtss_si32")
  • "test_mm_cvtt_ss2si" (alias for "mm_cvttss_si32")
  • "test_mm_cvt_si2ss" (alias for "mm_cvtsi32_ss")
  • "test_mm_set_ps1" (alias for "mm_set1_ps")

Each test follows the same pattern as the existing tests for the underlying
functions, since these aliases just call them directly.

Part of #798.

@rustbot
Copy link
Collaborator

rustbot commented Feb 26, 2026

r? @folkertdev

rustbot has assigned @folkertdev.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @Amanieu, @folkertdev, @sayantn
  • @Amanieu, @folkertdev, @sayantn expanded to Amanieu, folkertdev, sayantn
  • Random selection from Amanieu, folkertdev, sayantn

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.

Could you remove these lines too please

"_mm_cvt_ss2si",
"_mm_cvtt_ss2si",
"_mm_cvt_si2ss",
"_mm_set_ps1",

View changes since this review

@sayantn
Copy link
Contributor

sayantn commented Feb 26, 2026

Also, personally, I think it is a bit weird (and error-prone) to fully repeat the tests for aliases, considering there are quite a few of them. I would prefer a macro-based approach, but take this as a completely personal opinion.

@ArunTamil21
Copy link
Contributor Author

@sayantn Done, removed the lines from x86-intel.rs.

Regarding the macro-based approach, I agree it's cleaner.
Would you like me to refactor the tests in this PR, or should
I do that as a separate follow-up PR?

@sayantn
Copy link
Contributor

sayantn commented Feb 26, 2026

I would prefer it to be a single PR, as this PR just copying the tests anyway. If you prefer you can keep them as different commits

@ArunTamil21
Copy link
Contributor Author

@sayantn Thanks for the feedback. I'll refactor the tests using a macro-based approach.
Could you point me to an example of how you'd prefer the macro to look ?
I tried to check the sse.rs / sse2.rs couldn't find any macros based pattern.

@sayantn
Copy link
Contributor

sayantn commented Feb 26, 2026

I don't think we use a macro like that exactly, but you can make a simple macro like

macro_rules! foo_gen {
    ($($test_name:ident : $alias:ident),*) => {$(
        #[simd_test(blah)]
        fn $test_name() {
            //<< the body, but with $alias as the function name >>
        }
    )*}
}

foo_gen(foo, foo_alias);

or you can create a "meta function" like

unsafe fn test_foo_gen(actual_fn: unsafe fn()) {
    // << the body, but using `actual_fn` in place of that >>
}

#[simd_test(blah)]
fn test_foo() {
    test_foo_gen(foo);
}

#[simd_test(blah)]
fn test_foo_alias() {
    test_foo_gen(foo_alias);
}

any of these work, or if you can think of a better approach that's even better

…s for _mm_undefined_ps, _mm_prefetch, _mm_load_ps1, _mm_store_ps1
@rustbot
Copy link
Collaborator

rustbot commented Feb 26, 2026

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.

@ArunTamil21
Copy link
Contributor Author

@sayantn
I've refactored all the alias tests using the meta function pattern as suggested. Each pair of alias intrinsics now shares a single impl function to avoid code duplication.
I also added the missing tests for _mm_load_ps1, _mm_store_ps1, _mm_undefined_ps, and _mm_prefetch as part of issue #798.
For the last two, since they don't produce verifiable output per Intel's docs (prefetch only affects cache behavior, and _mm_undefined_ps returns indeterminate elements), the tests just verify that the functions compile and execute without crashing. Added a comment in each test explaining why.
Let me know if anything needs adjustment!

@sayantn
Copy link
Contributor

sayantn commented Feb 27, 2026

Thanks, looks nice. But I don't think we need tests for prefetch and undefined, we already have instruction tests to ensure that they produce the correct assembly

Already verified by assert_instr; no output to assert at runtime.
@ArunTamil21
Copy link
Contributor Author

Thanks @sayantn for the clarification — removed both. Good to know assert_instr already covers correctness for intrinsics with no verifiable output.

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