Deprecate fallback elsize for AbstractArray #26379
Conversation
Just like the deprecation of strides (#25321), the default implementation for `elsize` is generally not true. This deprecates the fallback, restructures it to be defined on the type (and not the value), and implements it where possible for the builtin array types and wrappers.
mbauman
added
arrays
| @@ -44,6 +44,7 @@ function size(a::ReinterpretArray{T,N,S} where {N}) where {T,S} | |||
| tuple(size1, tail(psize)...) | |||
| end | |||
|
|
|||
| elsize(::Type{<:ReinterpretArray{T}}) where {T} = sizeof(T) | |||
There was a problem hiding this comment.
This doesn't seem totally safe --- it seems like the parent array type would need to be involved somehow.
There was a problem hiding this comment.
There's no relation imposed by this code between the memory layout of a reinterpret array and its parent. However, it does impose that T is a known isbits type, which has a known memory size (although not stride) equal to sizeof(T)
There was a problem hiding this comment.
I think Jeff is right. Am I correct in thinking about the three cases here?
sizeof(eltype(R)) < sizeof(eltype(R.parent)): theelsizeof the parent should appear inReshapedArray's second stride.sizeof(eltype(R)) == sizeof(eltype(R.parent)):elsizeandstridesshould matchsizeof(eltype(R)) > sizeof(eltype(R.parent)): The reshaped array is only a strided array ifsizeof(eltype(R.parent)) == elsize(R.parent). We cannot pass this object to BLAS otherwise, since BLAS will assume that the bits are contiguous.
Edit: I'm not — I had been thinking that reinterpret added a dimension (which would be nice to save a divrem), but it cannot because it supports non-integral ratios between the two elsizes (so long as the total first dimension works out to an integer).
There was a problem hiding this comment.
Note that I'm going to follow this up with the strides portion of #26072, which will include part of that. I'm not sure how to handle that third bullet point, though.
There was a problem hiding this comment.
I guess we should maybe merge this now anyway, since it's no worse than it was before.
There was a problem hiding this comment.
It's less of a problem now that #26393 is merged — only reinterprets of DenseArrays (and contiguous views thereof) are considered strided now. We'll have to be very careful about how ReinterpretArrays opt-in to being considered strided in the future. I think we'll just need to demand that elsize(parent(R)) == sizeof(eltype(parent(R))) and stride(parent(R), 1) == 1. In all other cases, this cannot be correct.
Just like the deprecation of strides (#25321), the default implementation for
elsizeis generally not true. This deprecates the fallback, restructures it to be defined on the type (and not the value), and implements it where possible for the builtin array types and wrappers.