Skip to content

stdarch-test: various cleanups #1860

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

My goal here was to make the instruction matching stricter, to prevent accepting e.g. fminnm when the test specifies the fmin instruction must be used.

But especially on x86 there are lots of cases where a prefix is (deliberately?) used.

Anyway, I also made some other cleanups. Can be reviewed commit-by-commit.

@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 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

@folkertdev folkertdev force-pushed the stdarch-test-cleanup branch from 77ccca1 to ded4a23 Compare July 14, 2025 23:32
@folkertdev folkertdev force-pushed the stdarch-test-cleanup branch from ded4a23 to 2bcb6a2 Compare July 15, 2025 20:17
@folkertdev
Copy link
Contributor Author

I've updated the logic to use is_ascii_alphanumeric, though it's still commented out.

A selection of the false positives:

for x86

---- core_arch::x86::avx::assert__mm256_store_pd_vmovap stdout ----
disassembly for stdarch_test_shim__mm256_store_pd_vmovap: 
	 0: vmovaps %ymm0,(%rdi)
	 1: vzeroupper
	 2: ret

thread 'core_arch::x86::avx::assert__mm256_store_pd_vmovap' panicked at crates/stdarch-test/src/lib.rs:204:9:
failed to find instruction `vmovap` in the disassembly

---- core_arch::x86::avx::assert__mm256_storeu_pd_vmovup stdout ----
disassembly for stdarch_test_shim__mm256_storeu_pd_vmovup: 
	 0: vmovups %ymm0,(%rdi)
	 1: vzeroupper
	 2: ret

thread 'core_arch::x86::avx::assert__mm256_storeu_pd_vmovup' panicked at crates/stdarch-test/src/lib.rs:204:9:
failed to find instruction `vmovup` in the disassembly

---- core_arch::x86::fma::assert__mm_fnmsub_ss_vfnmsub stdout ----
disassembly for stdarch_test_shim__mm_fnmsub_ss_vfnmsub: 
	 0: vfnmsub213ss %xmm2,%xmm1,%xmm0
	 1: ret

thread 'core_arch::x86::fma::assert__mm_fnmsub_ss_vfnmsub' panicked at crates/stdarch-test/src/lib.rs:204:9:
failed to find instruction `vfnmsub` in the disassembly

---- core_arch::x86::pclmulqdq::assert__mm_clmulepi64_si128_pclmul stdout ----
disassembly for stdarch_test_shim__mm_clmulepi64_si128_pclmul: 
	 0: pclmullqlqdq %xmm1,%xmm0
	 1: ret

thread 'core_arch::x86::pclmulqdq::assert__mm_clmulepi64_si128_pclmul' panicked at crates/stdarch-test/src/lib.rs:204:9:
failed to find instruction `pclmul` in the disassembly

for aarch64 I was thinking of zip1/zip2. I've now fixed that but there are a bunch that remain

---- core_arch::aarch64::neon::generated::assert_vmull_high_p64_pmull stdout ----
disassembly for stdarch_test_shim_vmull_high_p64_pmull: 
	 0: pmull2 v0.1q, v0.2d, v1.2d
	 1: mov x1, v0.d[1]
	 2: fmov x0, d0
	 3: ret

thread 'core_arch::aarch64::neon::generated::assert_vmull_high_p64_pmull' panicked at crates/stdarch-test/src/lib.rs:204:9:
failed to find instruction `pmull` in the disassembly

---- core_arch::aarch64::neon::generated::assert_vmull_high_p8_pmull stdout ----
disassembly for stdarch_test_shim_vmull_high_p8_pmull: 
	 0: pmull2 v0.8h, v0.16b, v1.16b
	 1: ret

thread 'core_arch::aarch64::neon::generated::assert_vmull_high_p8_pmull' panicked at crates/stdarch-test/src/lib.rs:204:9:
failed to find instruction `pmull` in the disassembly

it looks like for some instructions, the 2 version is used for some types/widths. So checking for the exact instruction would mean a bunch of duplication in the spec document.

Perhaps we can extend the macro to accept an explicit prefix = "foo" or similar, so that the default is to perform a strict compare, but we can opt-in into a less strict match.

@Amanieu
Copy link
Member

Amanieu commented Jul 16, 2025

it looks like for some instructions, the 2 version is used for some types/widths. So checking for the exact instruction would mean a bunch of duplication in the spec document.

The 2 version is used for the _high versions of the intrinsics which operate on the high half instead of the low half of a SIMD register for narrowing/widening operations. So it does make sense to check for it.

Perhaps we can extend the macro to accept an explicit prefix = "foo" or similar, so that the default is to perform a strict compare, but we can opt-in into a less strict match.

That would be a good idea.

for x86

The store operations only involve data movement and could probably have the instruction check removed.

@folkertdev folkertdev force-pushed the stdarch-test-cleanup branch from 2bcb6a2 to 91c95eb Compare July 17, 2025 00:12
@folkertdev
Copy link
Contributor Author

Yeah, well I fixed all aarch64 issues, but for x86_64 it's a lot

test result: FAILED. 11021 passed; 898 failed;

so eh I'd propose we merge this now, and then we'll see about fixing those

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.

3 participants