LaihoE / SIMD-itertools

Faster implementations of standard library operations like find, filter, position etc.
168 stars 3 forks source link

`eq_simd` is broken #2

Closed EbbDrop closed 4 months ago

EbbDrop commented 4 months ago

It is possible for eq_simd to return false for two slices with the same values and same length.

I made some repo code:

#![feature(portable_simd)]

use simd_itertools::EqSimd;

fn main() {
    let a = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13];
    let b = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13];

    println!("a: {:?}", a.as_simd::<8>());
    println!("b: {:?}", b.as_simd::<8>());

    let expected = a.into_iter().eq(b);
    let got = a.iter().eq_simd(&b.iter());
    assert!(expected);
    assert_eq!(expected, got);  // <-- Panics here
}

but its very fragile, just removing the assert!(expected); stops it from panicking for me. I think the problem is that slice::as_simd() is allowed to split the two slices with the same values differently.

LaihoE commented 4 months ago

Fixed in 0.2.2

Yeah seems like if the a and b don't have the same alignment we get different splits. Stupid oversight from me when porting from exact_chunks to the "as_simd".

I find it odd that the test didn't catch it :thinking: maybe the test data is generated in a way that the alignment is always the same :thinking:

EbbDrop commented 4 months ago

Nice quick fix! I think the problem with the tests is that both arrays are independently randomly generated, so they are almost never equal, and this bug could only generate false negatives.