Fix handling of zero-size arrays #21
Conversation
* Patch matrixmultiply for zero-sized arrays See bluss/matrixmultiply#21. * Fix test_covariance_zero_observations The dot product should be all zeros, and the `dof` is `0. - (-1.) = 1.`, so all elements in the result should be `0. / 1. = 0.`. This matches the result from NumPy.
| @@ -115,6 +115,20 @@ unsafe fn gemm_loop<K>( | |||
| where K: GemmKernel | |||
| { | |||
| debug_assert!(m * n == 0 || (rsc != 0 && csc != 0)); | |||
| if m == 0 || k == 0 || n == 0 { | |||
There was a problem hiding this comment.
Thanks for finding this! I think this should be just "if k == 0" ? That's the case where the C matrix can have more than 0 elements. And a comment, // if A or B have no elements, compute C ← βC and return
Benchmarks pass for me with this PR, so it should be fine as it is.
There was a problem hiding this comment.
I added the comment.
Yeah, the m == 0 and n == 0 conditions aren't necessary, but I added them for a few reasons:
- The rest of the implementation no longer has to worry about
mornbeing zero. - For people reviewing the code, it's easy to see that this block works correctly for
m == 0andn == 0, while it's less obvious that's true for the rest of the implementation. - For these cases, it's nice to finish early instead of continuing to the rest of the implementation.
I can remove m == 0 and n == 0 if you'd prefer, though.
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.)
jturner314
force-pushed
the
fix-zero-sized
branch
from
344f4b4 to
96d5ea5
Compare
|
@jturner314 is it valid to have C a null pointer just because it is an empty matrix? I'm not sure |
Without the With the |
|
@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. |
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 wherem == 0 && n > 0and thecpointer is a null pointer (which is valid because thecmatrix is empty). Then, the callc.stride_offset(csc, knc * l5)was undefined behavior.)