BUG: Off by one in memory overlap check#26972
Conversation
a329ef2 to
d19ae28
Compare
seberg
left a comment
There was a problem hiding this comment.
Thanks! I do think we should add a test reverse strides arr[::-1] (all four combinations of it).
I like passing on len, that seems like a nice clean solution. One suggestion for the core change, but not needed, it does look right.
|
Not important for this PR, but for general awareness: NumPy ufuncs are actually protected against overlap with the exception of the |
The test fails when compiling on clang with "-ffp-contract=off", due to a combination of factors: * Difference between SIMD and non-SIMD loops * nomemoverlap falling back to non-SIMD loop due to off-by-one bug
The original check (since introduced in 085cdbe) returned overlap for adjacent arrays. This fixes one of the factors causing test_repeated_square_consistency failure, so the test should now always pass.
d19ae28 to
9d68e6e
Compare
|
Thanks @yairchu, let's put it in. In case anyone ever comes back to it: The test is somewhat interesting, but doesn't guarantee much unfortunately. To really test this, you would need a unit-test that gives different results for SIMD vs. non-SIMD paths (and even then, you cannot really test memory overlapping part easily). |
|
EDIT: I posted something incorrect here. |
An off-by-one in the
nomemoverlapcheck caused heisenbugs in platforms where the SIMD and non-SIMD code paths produced different results, due to arrays which semi-randomly got allocated adjacent to the input array failed the test and used the uncommon non-SIMD fallback.This PR was originally part of #26956, but split at the request of @seberg as well as fixed after a bug he noticed in the originally suggested fix.