BUG: longdouble with elsize 12 is never uint alignable #12611
Conversation
|
Commit 1 succeeded - the 32 bit test run crashed. Fix added for |
BUG: non-uint-aligned arrays were counted as uint-aligned
|
@ahaldane The end result is that any dtype with a non-power-of-two |
| return 1; | ||
| } | ||
| else { | ||
| /* always return false for alignment == 0, which means unaligned */ | ||
| return 0; |
There was a problem hiding this comment.
Why is alignment == 0 even a legal argument to this function?
There was a problem hiding this comment.
Maybe there is a better way to organize it... but the idea is that npy_uint_alignment, which normally returns the uint alignment, returns 0 if the type is not uint-alignable.
Then, it turned out to be convenient to allow the result of npy_uint_alignment to be passed directly to raw_array_is_aligned.
An alternative would be to check the return value of npy_uint_alignment explicitly everywhere it is used. But since it is pretty much always used as an arg to raw_array_is_aligned it seemed less complicated this way.
numpy/core/src/common/array_assign.h
Outdated
| @@ -88,7 +88,8 @@ broadcast_strides(int ndim, npy_intp *shape, | |||
| /* | |||
| * Checks whether a data pointer + set of strides refers to a raw | |||
| * array whose elements are all aligned to a given alignment. | |||
| * alignment should be a power of two. | |||
| * alignment should be a power of two, or may be the sentinel value 0 to mean | |||
| * unaligned, in which case false is always returned. | |||
There was a problem hiding this comment.
This comment confuses me - I read this as saying that is_aligned(..., alignment = 0) means is_unaligned(...)
There was a problem hiding this comment.
If I am not misreading you, that is what the comment is meant to say. If you pass an alignment of 0, the function always returns "not aligned". Do you mean I should say "not aligned" instead of "unaligned"?
There was a problem hiding this comment.
Right. the return value means "not aligned", but the meaning of the alignment argument is closer to "unalignable"
That's the idea, except to be precise it will be non-uint aligned. The logic is that the aligned copy code in Actually, it now occurs to me this might be relaxed a bit: An elsize of 12 could be uint-aligned to 4 bytes, so we could use a |
Let's leave this as a bugfix, and think about enhancements for another PR. |
That sounds like a good idea to me. |
|
Can we add a test case for this in any easy way? Does the |
|
I don't see where we expose the result of |
|
Nope, what you have is fine - I missed that the 32-bit build now catches the problem. Was just wondering if there's a platform-agnostic way this bug manifested itself. |
|
Thanks Matti, Allan. |
|
It is easier to backport these things if (internal) merges are done fast forward. Putting the test last, or at least squashed with the fix, is also helpful if a bisect needs to be done in the future. |
|
I think the merge was done automatically by github on Matti's github repo which I made a PR to. I'm not sure what the best way is for us to collaborate on PRs. |
|
During the work on |
Fix #12607
It seems we hit an alignment assert on 32bit linux with longdouble dtype (which has elsize 12). Fix it in two stages (the first commit only extends CI to test asserts):
-UNDEBUGto theCFLAGSso that the assert statements are checked (debug mode)