BUG: Fix simd loadable stride logic #27027
Merged
charris merged 7 commits intonumpy:mainfrom Aug 1, 2024
Merged
Conversation
It was never correct that strides are divisable by their itemsize and *some* code even checked for it correctly, while others had asserts in place that had no reason why they should be guaranteed to pass. This moves the check into the `loadable_stride` macro. The exp implementation has it's own custom logic, that is probably OK. I assume that compilers will optimize the duplicate division away, if that may not be the case, the code should be optimized to avoid it probably.
Member
Author
|
I have confirmed that this fixes the issue in the i386 docker env. It should be carefully reviewed anyway, since I am not sure how good the tests really are. The tests at gh-27028 crashed (presumably hitting one of the deleted asserts). EDIT: After removing the asserts, they failed beautifully. |
Member
|
Does this need a backport? There are a couple of other SIMD fixes... |
Member
Author
|
It's a huge bug, but I dunno, since it is also a larger change, and I think around for a while... |
Member
nice |
Member
The code looks good to me. Could you benchmark a relevant ufunc to see if there is any impact? |
Member
You mean here? Perhaps, we should use the same logic everywhere for consistency? |
r-devulap
approved these changes
Jul 30, 2024
| return MAXLOAD > 0 ? llabs(stride) <= MAXLOAD : 1; \ | ||
| } \ | ||
| NPY_FINLINE int \ | ||
| npyv_storable_stride_##SFX(npy_intp stride) \ |
Member
There was a problem hiding this comment.
Wondering if these can fold into one function. It duplicates the same logic.
Member
Author
There was a problem hiding this comment.
I can fold it into a single macro. Although, can MAXLOAD and MAXSTORE be reasonably different? Maybe it should just be NPY_SIMD_MAX_STRIDE?
Member
There was a problem hiding this comment.
As of now, only avx512 defines MAXSTORE and it is equal to MAXLOAD. Let's fold into a single value. There was no magic way to calculate them, IIRC it was a simple heuristic.
Member
Author
There was a problem hiding this comment.
Actually, AVX2 defines only the load version. I might suspect that it is quite silly to not just use that for the store either way (also stores are much less likely to have huge strides).
But, it makes me want to not include the change here.
Member
Author
|
Not sure there is a test that really might notice it (unless we disable simd accidentally), but I see these timings. That is huge fluctuations, but I can't say any of them look systematic (locally on mac). The only one that seems a bit plausible might be the 3rd one: Details |
Member
|
@seberg Are you OK with merging this as is? |
Member
Author
|
Yes, of course, was just giving reviewers a chance to comment in case. |
Member
|
Thanks Sebastian. |
Member
Author
|
Hmmmpf, the test didn't fail here but does not on other CI runs. I guess machine capability fluctuations... @r-devulap do you have time to have a look at fixing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It was never correct that strides are divisable by their itemsize and
some code even checked for it correctly, while others had asserts
in place that had no reason why they should be guaranteed to pass.
This moves the check into the
loadable_stridemacro. The expimplementation has it's own custom logic, that is probably OK.
I assume that compilers will optimize the duplicate division away,
if that may not be the case, the code should be optimized to avoid
it probably.
(although, I guess these are power of 2 divisions so they don't matter actually)
Closes gh-26775