Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Clipping code assumes alignment % itemsize == 0 #26341

Closed
seberg opened this issue Apr 24, 2024 · 4 comments
Closed

BUG: Clipping code assumes alignment % itemsize == 0 #26341

seberg opened this issue Apr 24, 2024 · 4 comments

Comments

@seberg
Copy link
Member

seberg commented Apr 24, 2024

The clipping code contains lines like:

        npy_intp is1 = steps[0] / sizeof(T), os1 = steps[3] / sizeof(T);

NumPy guarantees that ufunc data is aligned, but NumPy does not guarantee that alignment is equal to the itemsize. (in particularly, that is clearly not true for complex numbers)

As the quick fix: The code shouldn't use this pattern at all, I think.

But even if we ignore complex (because clipping and complex are weird), do platforms even guarantee that for the basic numerical types? Should code at least have a static_assert() or so if using such a pattern?

(xref gh-26280 because that is why I noticed)

@pijyoi
Copy link
Contributor

pijyoi commented Apr 28, 2024

NumPy guarantees that ufunc data is aligned, but NumPy does not guarantee that alignment is equal to the itemsize.

My understanding of the code in question is that it is only assuming that items are spaced in multiples of the itemsize. i.e. steps[0] % sizeof(T) == 0
If the starting pointer was aligned, then all subsequent accesses would also be aligned.

Does NumPy not provide those guarantees on entry to the clip function?
If not, it wouldn't even be safe to dereference elements on processors that don't allow misaligned access?

@seberg
Copy link
Member Author

seberg commented Apr 28, 2024

it wouldn't even be safe to dereference elements on processors that don't allow misaligned access

No, it guarantees alignment, which is good enough for that. But alignment can be smaller than itemsize.

@mhvk
Copy link
Contributor

mhvk commented Apr 28, 2024

But alignment can be smaller than itemsize.

So the above code would also fail for longdouble (assuming alignment on 8 byte boundaries).

I agree we should avoid the pattern altogether, especially since for most other ufuncs, we explicitly keep the pointers and just add the step to it.

@mhvk mhvk changed the title BUG: Clipping code assumes itemsize == alignment BUG: Clipping code assumes alignment % itemsize == 0 Apr 28, 2024
@seberg
Copy link
Member Author

seberg commented May 6, 2024

Closing, the speedup PR included the fix.

@seberg seberg closed this as completed May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants