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 handling of zero-size arrays #21

Closed jturner314 closed 5 years ago

jturner314 commented 6 years ago

Before, if m > 0 && n > 0 && k == 0, the result was never initialized or calculated. Additionally, in some other cases, pointer offsets could result in undefined behavior due to violating the constraints on the .offset() method. (For example, consider the case where m == 0 && n > 0 and the c pointer is a null pointer (which is valid because the c matrix is empty). Then, the call c.stride_offset(csc, knc * l5) was undefined behavior.)

bluss commented 5 years ago

@jturner314 is it valid to have C a null pointer just because it is an empty matrix? I'm not sure

jturner314 commented 5 years ago

is it valid to have C a null pointer just because it is an empty matrix?

Without the m == 0 condition in this PR, the answer is "no" according to the docs of offset because of this line. However, according to the LLVM docs for getelementptr inbounds (offset is equivalent to getelementptr inbounds), it is safe to offset a null pointer by zero bytes, so in practice, it should be fine.

With the m == 0 and n == 0 constraints in this PR, though, a null pointer is clearly safe because it's never offset, dereferenced, etc.

bluss commented 5 years ago

@jturner314 thanks again for this nice fix. I'd like to say that just because the code allows nulls in this state, I'm not sure it's a good idea to "open up" for that possibility, I'd prefer if we required the pointers were non-null.