Mr/guard simd tests

[Fabian-Gruenbichler]

the neon and avx512 adler32 tests currently require the std feature being enabled to work, since the CPU feature detection is based on it.

note that there is a slight inconsistency between the two test modules:

• neon assumes the target has the feature enabled
• avx tests only run if the target has it enabled

the former means that if the target is compiled without neon support, the tests would still fail. the latter means that the tests don't run on targets that don't have avx enabled, but could support it at runtime if the CPU does (e.g., Debian's amd64 doesn't have AVX enabled automatically, but most modern CPUs support it)..

ideally, the runtime detection would just cause those tests to be skipped, instead of making them fail..

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
fuzz-compress	?
fuzz-decompress	?
test-aarch64-apple-darwin	92.26% <ø> (+0.01%)	⬆️
test-aarch64-unknown-linux-gnu	89.71% <ø> (ø)
test-i686-unknown-linux-gnu	89.50% <ø> (+0.02%)	⬆️
test-x86_64-apple-darwin	91.55% <ø> (-0.02%)	⬇️
test-x86_64-unknown-linux-gnu	92.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
zlib-rs/src/adler32/avx2.rs	92.53% <ø> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bjorn3]

The aarch64 targets enable neon, so #[cfg(target_feature = "neon")] seems like it would be sufficient if you add #[cfg(target_feature = "neon")] return true; to is_enabled_neon.

[Fabian-Gruenbichler]

that should also work, yes. I can rebase this later using that approach if you want!

[bjorn3]

The alternative would be to add #[cfg(all(target_feature = "bmi1", target_feature = "bmi2")], right?

[Fabian-Gruenbichler]

that would be more complete, but running the test with cargo test --no-default-features --features c-allocator (as an example) would still fail, because the runtime CPU feature detection will always return false if the std feature is not enabled.

[bjorn3]

It shouldn't. If avx2, bmi1 and bmi2 are enabled at compile time then is_enabled_avx2_and_bmi2 will unconditionally return true even when the std feature` is disabled.

zlib-rs/zlib-rs/src/cpu_features.rs

Lines 33 to 38 in bbf2574

	#[cfg(all(
	target_feature = "avx2",
	target_feature = "bmi1",
	target_feature = "bmi2"
	))]
	return true;

[Fabian-Gruenbichler]

ah right, I looked at the wrong runtime checker (the one for avx512, not avx2_..).

[Fabian-Gruenbichler]

I'll try to expand a bit:

• this MR just fixes running cargo test --no-default-features --features XX where XX doesn't directly or indirectly enable the std feature
• there's a second related issue that the neon tests will still fail if a target has the feature compiled in, but the CPU doesn't actually have it (which might be fine, this is an invalid combination after all)
• there's a third related issue that the avx tests will not be run if a target doesn't have the feature compiled in, but the CPU does actually have it (which just reduces test coverage)

[folkertdev]

Can we replicate this issue with cross-compilation? neon cannot actually be disabled for the aarch64 targets I think. I'm also not having any luck reproducing this for x86_64.

I'm a bit worried that we will at some point forget to include the std feature somewhere and miss test coverage that we thought we would have.

[Fabian-Gruenbichler]

cross compilation doesn't help here, you need to execute the tests to see the failures. I verified the issue on an arm64 porterbox for the neon part. the avx part requires a target that has avx compiled in, or removing the #[cfg(target_feature = "avx2")] line, to mimic how neon is handled atm:

$ git diff
diff --git a/zlib-rs/src/adler32/avx2.rs b/zlib-rs/src/adler32/avx2.rs
index 04b14a4..fa574ff 100644
--- a/zlib-rs/src/adler32/avx2.rs
+++ b/zlib-rs/src/adler32/avx2.rs
@@ -145,7 +145,6 @@ unsafe fn helper_32_bytes(mut adler0: u32, mut adler1: u32, src: &[__m256i]) ->
 }

 #[cfg(test)]
-#[cfg(target_feature = "avx2")]
 mod test {
     use super::*;


$ cargo test -p zlib-rs --no-default-features --features=c-allocator 
running 75 tests
test adler32::avx2::test::start_alignment ... FAILED
test adler32::avx2::test::large_input ... FAILED
test adler32::avx2::test::empty_input ... FAILED
test adler32::test::naive_is_fancy_small_inputs ... ok
test allocate::tests::test_allocate_buffer_zeroed ... ok
test allocate::tests::test_allocate_zeroed ... ok
test adler32::avx2::test::adler32_avx2_is_adler32_rust ... FAILED
test allocate::tests::test_deallocate_null ... ok
test adler32::test::test_adler32_combine ... ok
test allocate::tests::unaligned_allocator_0 ... ok
test allocate::tests::unaligned_allocator_16 ... ok
test allocate::tests::unaligned_allocator_2 ... ok
test allocate::tests::unaligned_allocator_8 ... ok
test crc32::braid::test::crc32_naive_inner_endianness_and_alignment ... ok
test crc32::braid::test::empty_is_identity ... ok
test allocate::tests::unaligned_allocator_32 ... ok
test allocate::tests::unaligned_allocator_64 ... ok
test crc32::braid::test::naive_is_crc32fast ... ok
test crc32::braid::test::words_endianness ... ok
test allocate::tests::unaligned_allocator_4 ... ok
test crc32::braid::test::braid_6_is_crc32fast ... ok
test deflate::hash_calc::tests::standard_hash_calc ... ok
test allocate::tests::unaligned_allocator_1 ... ok
test deflate::slide_hash::tests::test_slide_hash_avx2 ... ok
test crc32::braid::test::braid_5_is_crc32fast ... ok
test deflate::slide_hash::tests::test_slide_hash_rust ... ok
test deflate::slide_hash::tests::slide_is_rust_slide ... ok
test crc32::braid::test::words_is_crc32fast ... ok
test crc32::braid::test::braid_4_is_crc32fast ... ok
test crc32::combine::test::test_crc32_combine ... ok
test crc32::test::nasty_alignment ... ok
test crc32::test::test_crc32_fold ... ok
test deflate::test::block_flush ... ok
test crc32::test::test_crc32_fold_align ... ok
test deflate::compare256::avx2::test_compare256 ... ok
test deflate::compare256::rust::test_compare256 ... ok
test deflate::compare256::rust::test_compare256_rle ... ok
test deflate::test::copy_invalid_allocator::fail_0 ... ok
test deflate::hash_calc::tests::roll_hash_calc ... ok
test crc32::test::chunked ... ok
test deflate::test::copy_invalid_allocator::fail_5 ... ok
test deflate::test::copy_invalid_allocator::fail_3 ... ok
test deflate::test::detect_data_type_basic ... ok
test crc32::test::crc_fold_is_crc32fast ... ok
test deflate::test::from_stream_mut ... ok
test deflate::test::full_flush ... ok
test deflate::test::fill_window_out_of_bounds ... ok
test deflate::test::gzip_header_pending_flush ... ok
test deflate::test::inflate_window_copy_slice ... ok
test deflate::test::hello_world_huffman_only ... ok
test deflate::test::gzip_stored_block_checksum ... ok
test deflate::test::invalid_deflate_config::window_bits_too_high ... ok
test deflate::test::invalid_deflate_config::window_bits_too_low ... ok
test deflate::test::hash_calc_difference ... ok
test deflate::test::partial_flush ... ok
test deflate::test::insufficient_compress_space ... ok
test deflate::test::gzip_no_header ... ok
test deflate::test::hello_world_quick_random ... ok
test deflate::test::init_invalid_allocator ... ok
test deflate::test::gzip_with_header ... ok
test inflate::inftrees::test::not_enough_errors ... ok
test inflate::tests::uncompress_buffer_overflow ... ok
test deflate::test::invalid_deflate_config::window_bits_correction ... ok
test inflate::writer::test::copy_chunk_unchecked ... ok
test deflate::test::simple_rle ... ok
test inflate::writer::test::copy_match_insufficient_space_for_simd ... ok
test deflate::test::invalid_deflate_config::sanity_check ... ok
test deflate::test::end_data_error ... ok
test deflate::test::test_reset_keep ... ok
test deflate::test::split_deflate ... ok
test inflate::inftrees::test::generate_fixed_distance_table ... ok
test inflate::writer::test::copy_match ... ok
test deflate::test::sync_flush ... ok
test inflate::inftrees::test::generate_fixed_length_table ... ok
test deflate::test::hello_world_quick ... ok

failures:

---- adler32::avx2::test::start_alignment stdout ----

thread 'adler32::avx2::test::start_alignment' panicked at zlib-rs/src/adler32/avx2.rs:68:5:
assertion failed: crate::cpu_features::is_enabled_avx2_and_bmi2()

---- adler32::avx2::test::large_input stdout ----

thread 'adler32::avx2::test::large_input' panicked at zlib-rs/src/adler32/avx2.rs:68:5:
assertion failed: crate::cpu_features::is_enabled_avx2_and_bmi2()

---- adler32::avx2::test::empty_input stdout ----

thread 'adler32::avx2::test::empty_input' panicked at zlib-rs/src/adler32/avx2.rs:68:5:
assertion failed: crate::cpu_features::is_enabled_avx2_and_bmi2()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- adler32::avx2::test::adler32_avx2_is_adler32_rust stdout ----

thread 'adler32::avx2::test::adler32_avx2_is_adler32_rust' panicked at zlib-rs/src/adler32/avx2.rs:68:5:
assertion failed: crate::cpu_features::is_enabled_avx2_and_bmi2()

thread 'adler32::avx2::test::adler32_avx2_is_adler32_rust' panicked at zlib-rs/src/adler32/avx2.rs:68:5:
assertion failed: crate::cpu_features::is_enabled_avx2_and_bmi2()

thread 'adler32::avx2::test::adler32_avx2_is_adler32_rust' panicked at zlib-rs/src/adler32/avx2.rs:68:5:
assertion failed: crate::cpu_features::is_enabled_avx2_and_bmi2()

thread 'adler32::avx2::test::adler32_avx2_is_adler32_rust' panicked at /home/fgruenbichler/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quickcheck-1.0.3/src/tester.rs:165:28:
[quickcheck] TEST FAILED (runtime error). Arguments: ([], 0)
Error: "assertion failed: crate::cpu_features::is_enabled_avx2_and_bmi2()"


failures:
    adler32::avx2::test::adler32_avx2_is_adler32_rust
    adler32::avx2::test::empty_input
    adler32::avx2::test::large_input
    adler32::avx2::test::start_alignment

test result: FAILED. 71 passed; 4 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

the same test invocation with the std feature added (or --no-default-features dropped) works (unless your CPU is lacking AVX, in which case it will fail again ;))

[folkertdev]

Hmm then I think statically gating on the target features like bjorn suggested is the best approach. Yes we do lose some test coverage, but the functions at hand receive plenty of coverage via our dedicated tests. I don't think testing on a wider variety of hardware has much value. I don't think targets without these features are likely to be used for development, and in any case our CI would catch any issues.

(well I guess we should check github CI runners have bmi1/bmi2? I'd expect them to have it, but we should make sure)

[Fabian-Gruenbichler]

reworked the two commits:

• change the neon runtime detection code to check target_feature first, like the avx2 one
• adapt the avx test to require all three target features

[folkertdev]

Hmm we are actually losing coverage right now. One way to fix that is to build with +avx2,+bmi1,+bmi2 by adding

      CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS: "-Ctarget-feature=+avx2,+bmi1,+bmi2"

in the CI script, like here

zlib-rs/.github/workflows/checks.yaml

Lines 35 to 37 in bbf2574

	CARGO_TARGET_WASM32_WASIP1_RUNNER: "wasmtime --dir /home/runner/work/zlib-rs/zlib-rs/test-libz-rs-sys --dir /tmp/"
	CARGO_TARGET_WASM32_WASIP1_RUSTFLAGS: "-Ctarget-feature=+simd128"
	BINDGEN_EXTRA_CLANG_ARGS_wasm32_wasip1: "--sysroot=/tmp/wasi-sdk-24.0-x86_64-linux/share/wasi-sysroot"

we'd then run without these tests, but with dynamic feature detection, for our x86 windows and macos builds. That should be sufficient I think.

[Fabian-Gruenbichler]

do you want to add that yourself? I know very little about your CI setup, so you are probably in a better position to decide which target features to set for which of the test runs.. alternatively, I had another patch on top like this:

diff --git a/zlib-rs/src/adler32/avx2.rs b/zlib-rs/src/adler32/avx2.rs
index 7ea869f..e845aca 100644
--- a/zlib-rs/src/adler32/avx2.rs
+++ b/zlib-rs/src/adler32/avx2.rs
@@ -145,10 +145,13 @@ unsafe fn helper_32_bytes(mut adler0: u32, mut adler1: u32, src: &[__m256i]) ->
 }
 
 #[cfg(test)]
-#[cfg(all(
-    target_feature = "avx2",
-    target_feature = "bmi1",
-    target_feature = "bmi2"
+#[cfg(any(
+    all(
+        target_feature = "avx2",
+        target_feature = "bmi1",
+        target_feature = "bmi2"
+    ),
+    feature = "std"
 ))]
 mod test {
     use super::*;

that would re-enable the avx2 tests with runtime detection (and potential for failure if the avx2 and co features are not supported!) if the std feature is enabled..

[folkertdev]

I merged the changes to neon in a separate PR and rebased here.

That leaves the issue where tests of simd implementations fail when the required features are not available at runtime. I'm wondering though: why is that a problem? Does it cause issues with the debian processes?

From my perspective I'd really like the default settings to test as widely as possible on the most common targets.

[Fabian-Gruenbichler]

it's not an issue per se, if that is the behaviour you want upstream than that's obviously okay.

we'll patch around it downstream, because we don't want tests failing depending on which test environment they run in (e.g., between a developer's build environment and the official CI, or between different runners of the official CI).

unfortunately cargo test doesn't have the option (AFAIK?) for a test case to say at runtime "please treat me as skipped, I couldn't run", else that would be the best solution here I think.