Add ReinterpretArrays to supported array types

[BioTurboNick]

I added ReinterpretArrays to the supported contiguous array types. I think it should just work?

[KristofferC]

cc @tkf

[tkf]

I think it'd be nice to have some multi-dim test cases. Especially with FastContiguousArray.

[tkf]

@KristofferC The nightly failure is due to the change in juila. I guess we can ignore it for now since it would be fixed in #70?

[KristofferC]

Yes

[BioTurboNick]

I think it'd be nice to have some multi-dim test cases. Especially with FastContiguousArray.

Can you please clarify what you mean? It looks like vload and vstore are restricted to 1-dimensional arrays.

[tkf]

ContiguousArray and FastContiguousArray are used in getindex/setindex!. For example:

SIMD.jl/src/arrayops.jl

Lines 161 to 162 in b19dcde

	@propagate_inbounds Base.getindex(a::FastContiguousArray{T,1}, idx::Vec{N,Int}) where {N,T} =
	vgather(a, idx)

SIMD.jl/src/arrayops.jl

Lines 282 to 287 in b19dcde

	Base.@propagate_inbounds function Base.getindex(
	arr::ContiguousArray{T}, idx::VecRange{N},
	args::Vararg{Union{Integer,Vec{N,Bool}}}) where {N,T}
	I, mask = _preprocessindices(arr, idx, args)
	return vload(Vec{N,T}, _pointer(arr, idx.i, I), mask)
	end

Since we are now going to allow ReinterpretArray to hit those methods, I thought it'd be better to check them in the test.

[BioTurboNick]

@tkf - I apologize, I'm not entirely clear on how the indexing notation works. I can load/store normal and reinterpret 1-d arrays, but not sure how multidimensional should work.

a = UInt8[1:32...]
b = reinterpret(UInt32, a)
d = b[Vec(1,3)]
b[Vec(1,3)] = Vec{2, UInt32}((2, 4))

e = reshape(a, (8,4))
e[Vec(1, 3)] # ERROR: MethodError: no method matching vgather(::Vec{2,Ptr{UInt8}}, ::Nothing)

[tkf]

I was thinking something like

x = reinterpret(Float64, view(zeros(UInt8, 64, 8), :, 1:2:8))
x[VecRange{4}(1), 1]

can be tested now.

MethodError: no method matching vgather(::Vec{2,Ptr{UInt8}}, ::Nothing)

This sounds like a bug to me (maybe a pre-existing one triggered by this PR).

[tkf]

MethodError: no method matching vgather(::Vec{2,Ptr{UInt8}}, ::Nothing)

This sounds like a bug to me (maybe a pre-existing one triggered by this PR).

Actually, this is nothing to do with this PR. It's fixed in #73.

[codecov-commenter]

Codecov Report

Merging #72 into master will increase coverage by 0.73%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   82.23%   82.96%   +0.73%     
==========================================
  Files           5        5              
  Lines         653      681      +28     
==========================================
+ Hits          537      565      +28     
  Misses        116      116              

Impacted Files	Coverage Δ
src/arrayops.jl	95.96% <100.00%> (+0.51%)	⬆️
src/LLVM_intrinsics.jl	96.80% <0.00%> (+0.01%)	⬆️
src/simdvec.jl	78.28% <0.00%> (+1.74%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b19dcde...6877d5e. Read the comment docs.

[BioTurboNick]

Rebased onto master and added the multidimensional tests. Had to add _pointer methods to make it work, please make sure that looks okay.

[tkf]

I think it'd be better to merge them with above methods using Union{Array,Base.ReinterpretArray} and Union{Base.FastContiguousSubArray,Base.ReinterpretArray}.

[BioTurboNick]

Hmm, Union{Array, Base.ReinterpretArray} works okay, but Union{Base.FastContiguousSubArray, Base.ReinterpretArray} led to MethodError: _pointer(::SubArray{Int32,1,Array{Int32,1},Tuple{UnitRange{Int64}},true}, ::Int64, ::Tuple{}) is ambiguous. in the existing indexed vgather tests.

I don't understand why an ambiguity would exist. They should be equivalent expressions in Julia right? It's confused between:

_pointer(arr::SubArray{T,N,P,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where P where N where T, i, I) in SIMD at C:\Users\nicho\source\repos\SIMD.jl\src\arrayops.jl:279
_pointer(arr::Union{Base.ReinterpretArray, SubArray{T,N,P,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where P where N where T}, i, I::Tuple{}) in SIMD at C:\Users\nicho\source\repos\SIMD.jl\src\arrayops.jl:281

It seems that it may not be doing multiple dispatch correctly? This is on Julia 1.5.1

[tkf]

Hmm... That's unfortunate. I guess using Union makes the argument slot less specific. Something like

julia> f(::Union{Int32,Int64}, ::Tuple{}) = 1;

julia> f(::Int32, ::Any) = 2;

julia> f(::Int64, ::Any) = 3;

julia> f
f (generic function with 3 methods)

julia> f(1, ())
ERROR: MethodError: f(::Int64, ::Tuple{}) is ambiguous. Candidates:
  f(::Int64, ::Any) in Main at REPL[20]:1
  f(::Union{Int32, Int64}, ::Tuple{}) in Main at REPL[18]:1
Possible fix, define
  f(::Int64, ::Tuple{})

I guess it's better to keep _pointer(arr::Base.ReinterpretArray, i, I::Tuple{}) as-is, with maybe some comments saying that ReinterpretArray and FastContiguousSubArray are different method to avoid method ambiguity.

[tkf]

Since there is no API guarantee that reinterpret returns a ReinterpretArray (although I doubt Base would change it) and to be very explicit about the type of the variables, I think it's a good idea to add type assertions.

Other than these minor points, the PR looks good to me.

[BioTurboNick]

Do you think the PR can be merged?