MAINT: Further fixups to uint alignment checks #12677
Conversation
9229d8a to
667f093
Compare
| */ | ||
| if (IsUintAligned(op[iop])) { | ||
| op_itflags[iop] |= NPY_OP_ITFLAG_ALIGNED; | ||
| } |
There was a problem hiding this comment.
Are there "aligned optimizations" (as mentioned in the comment) beyond the call to PyArray_GetDTypeTransferFunction?
There was a problem hiding this comment.
No, grepping showed that it was only used there.
| * needs uint-alignment in addition. | ||
| */ | ||
| if (IsUintAligned(out)) { | ||
| op_itflags[iop] |= NPY_OP_ITFLAG_ALIGNED; |
There was a problem hiding this comment.
I'm confused why we don't simply revert to the code pre-#12626, and set OP_ITFLAG_ALIGNED unconditionally (and then use it in the transfer function if statements - it seems the least amount of churn and keeps the flag.
2886c88 to
9dd6bdb
Compare
|
The coverage test has gone bonkers. Might be related to the pytest 4.1.0 release in the last 22 hours. |
9dd6bdb to
f54f39f
Compare
|
@ahaldane Do you feel done with this? I can follow the code, but cannot judge its correctness without a bigger context. Perhaps some comments would help. EDIT: Particularly regarding big vs little alignment. I assume the use of the strides is to guarantee that alignment stays good while indexing through the array. |
|
I'm going to put this in for testing purposes. @ahaldane More documentation explaining the purpose of the new function would be desirable. |
|
Thanks Allan. |
|
@charris, will do. Just busy today. |
A few more fixes following on #12626, based on testing on a ppc64be system.
First, on this system some einsum tests failed due typo in that PR:
;should have been&&. This wasn't caught by x64 unit tests, so I've added a test that should cover those code paths.Second, I figured out that some of the fix in #12626 wasn't quite right: I had misunderstood what the
NPY_ITER_ALIGNEDflag is supposed to do. My understanding now is that is a request to guarantee that the nditer buffers are "true" aligned. (uint alignment is only an internal numpy thing which users shouldn't know about), which nditer it does by forcing a buffer/cast if that flag is requested, since all new buffers are true aligned. With that understanding, it now seems that theNPY_OP_ITFLAG_ALIGNEDflag is not needed: Its only use was as a precalculated computation of thealignedparameter ofPyArray_GetDTypeTransferFunction, but that is more safely computed on the spot, based on both uint and true alignment of the op. So I've removed all uses ofNPY_OP_ITFLAG_ALIGNED.This latter change was indirectly discovered from staring at the code while trying to bug-hunt failed alignment asserts on ppc64be. But those turned out to be irrelevant glibc problems from #10491. At least it got me to read a lot of code over more thoroughly: I also made sure that the ufunc code doesn't need alignment fixes.