search

bluss / matrixmultiply

General matrix multiplication of f32 and f64 matrices in Rust. Supports matrices with general strides.
https://docs.rs/matrixmultiply/
Apache License 2.0
209 stars 25 forks source link

Fix Miri error with -Zmiri-tag-raw-pointers #67

Closed jturner314 closed 2 years ago

jturner314 commented 2 years ago

Before this PR, running MIRIFLAGS="-Zmiri-tag-raw-pointers" cargo miri test caused Miri to report undefined behavior in the test_dgemm test. This PR fixes the underlying issue – Miri doesn't like us using a reference to an element to access other elements.

There may be other Miri errors. Miri takes forever to run, and I haven't waited for it to finish yet.

bluss commented 2 years ago

I need to run this through the benchmark. I guess it's fine of course. Thanks a lot.

5225225 commented 2 years ago

Miri takes forever to run, and I haven't waited for it to finish yet.

You could adjust the FAST_TEST in sgemm.rs to be

const FAST_TEST: bool = option_env!("MMTEST_FAST_TEST").is_some() || cfg!(miri);

which makes a full run take a minute on my machine

bluss commented 2 years ago

miri ci already sets MMTEST_FAST_TEST FWIW, that's why it's there.

jturner314 commented 2 years ago

Oh, that's much better. MMTEST_FAST_TEST=1 MIRIFLAGS="-Zmiri-tag-raw-pointers -Zmiri-check-number-validity" RUSTFLAGS="-Zrandomize-layout" cargo +nightly miri test runs in less than 20 seconds and has no errors (with this PR).

@bluss Would you like me to add MIRIFLAGS="-Zmiri-tag-raw-pointers -Zmiri-check-number-validity" RUSTFLAGS="-Zrandomize-layout" to ci/miri.sh?

bluss commented 2 years ago

yes why not

bluss commented 2 years ago

Thanks!