Add Equal() to indicate that offset1 equals first offset

[chriselrod]

Save a value if we cross a function boundary with OffsetVectors.

[codecov]

Codecov Report

Merging #189 (51f5ac5) into master (d57f72d) will increase coverage by 0.06%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     JuliaArrays/ArrayInterface.jl#189      +/-   ##
==========================================
+ Coverage   83.91%   83.98%   +0.06%     
==========================================
  Files          11       11              
  Lines        1610     1617       +7     
==========================================
+ Hits         1351     1358       +7     
  Misses        259      259              

Impacted Files	Coverage Δ
src/stridelayout.jl	88.69% <66.66%> (+0.08%)	⬆️
src/ArrayInterface.jl	80.05% <100.00%> (+0.22%)	⬆️
src/array_index.jl	97.40% <100.00%> (+0.01%)	⬆️

---

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 d57f72d...51f5ac5. Read the comment docs.

[Tokazama]

Would it make sense to have offset1 also account for offsets in SubArray (e.g., JuliaArrays/StaticArrayInterface.jl#11). Then the only case where offset1 wouldn't be Equal would be when it represents a SubArray.

[chriselrod]

I'd prefer not to have offset1 handle the SubArray's offset, because then I'd need to special case stridedpointer on SubArray to take the pointer of the parent.

I'm transitioning StridedPointers to use StrideIndex in LayoutPointers, and will then eventually have VectorizationBase depend on it.

EDIT:
I'm not sure if that's best though, as I generally want to represent strides in terms of bytes, which means I can't make use of the indexing code.

[Tokazama]

I'd prefer not to have offset1 handle the SubArray's offset, because then I'd need to special case stridedpointer on SubArray to take the pointer of the parent.

Isn't this done with memory_reference here? Side note, wouldn't you have PseudoPtr returned with the same offset when the device is CPUIndex?

If offset1(x) doesn't refer to the linear offset to the first value of x, then should the default definition be offset1(x) = getfield(offsets(x), 1, false) and we drop the offset1 field entirely from StrideIndex?

EDIT:
I'm not sure if that's best though, as I generally want to represent strides in terms of bytes, which means I can't make use of the indexing code.

I've been hacking on a set of ArrayIndex subtypes for ReshapedArray, PermutedDimsArray, SubArray, and ReinterpretArray. Then these can be arbitrarily combined (with something like combined_index(child_index, parent_index)). The first three are pretty straightforward but I think we'd need to have some way of representing N bytes per each index (which shouldn't be that hard, but I haven't had time to think through a proper solution all the way).

[Tokazama]

Sorry if my comment seems off topic. It seems to me that this PR is improving upon a redundancy (or ambiguity) of offset1 (the method and field of StrideIndex). I just want to be sure that we formalize what offset1 is before creating additional layers of complexity on top of it.

[chriselrod]

Isn't this done with memory_reference here? Side note, wouldn't you have PseudoPtr returned with the same offset when the device is CPUIndex?

Ha, forgot I was already special casing it.
So making that change would involve removing this special casing (so that memory_refernce looks to the parent), and then make sure I set its offsets correctly.

For me, it is easier not to make those changes. LV tries to zero the offsets, meaning incrementing the pointers, anyway, so w/ respect to LV, both versions should compile to the same code anway.
I think A[StrideIndex(A)[inds...]] == A[inds...] also makes sense, which we have from the current definition, while the new one would have parent(A)[StrideIndex(A)[inds...]] = A[inds...].

If offset1(x) doesn't refer to the linear offset to the first value of x, then should the default definition be offset1(x) = getfield(offsets(x), 1, false) and we drop the offset1 field entirely from StrideIndex?

IMO, unsafe_load(pointer(A)) == first(A), which is true for SubArrays because pointer (and memory_reference) offset the pointer when called so that this invariant holds.

Under the perspective that the base pointer of a SubArray is the pointer to its first element (rather than the pointer to the parent), then offset1 currently does return the linear offset.
And for A::OffsetArray{T,N} where N > 1, we do not have that offset1(A) == first(offsets(A)).

[chriselrod]

I've been hacking on a set of ArrayIndex subtypes for ReshapedArray, PermutedDimsArray, SubArray, and ReinterpretArray. Then these can be arbitrarily combined (with something like combined_index(child_index, parent_index)). The first three are pretty straightforward but I think we'd need to have some way of representing N bytes per each index (which shouldn't be that hard, but I haven't had time to think through a proper solution all the way).

If these arrays are all strided, I'd rather have a single representation capable of describing any of them. StrideIndex with rank R should be able to handle any of these (assuming they're what ArrayInterface considers to be StridedArrays).

[chriselrod]

Sorry if my comment seems off topic. It seems to me that this PR is improving upon a redundancy (or ambiguity) of offset1 (the method and field of StrideIndex). I just want to be sure that we formalize what offset1 is before creating additional layers of complexity on top of it.

I am happy with the current semantics, I just wanted to elliminate redundant data storage.
LoopVectorization tries to pass data in registers instead of on the stack, but the number of registers you can use for this is very limited.

That's also one of the motivations of the indices and groupedstridedpointer handling, to try and eliminate redundancies between sizes.

[Tokazama]

Neither A[StrideIndex(A)[inds...]] nor parent(A)[StrideIndex(A)[inds...] is technically the current way this functions.

For example, something we currently test with StrideIndex is this:

julia> A = zeros(3, 4, 5);

julia> A[:] = 1:60;

julia> Ap = @view(PermutedDimsArray(A,(3,1,2))[:,1:2,1])';

julia> ap_index = ArrayInterface.StrideIndex(Ap);

julia> ap_index[2,2] == 14 == Ap[2, 2]
true

julia> Ap[14]
ERROR: BoundsError: [...]

julia> parent(Ap)[14]
ERROR: BoundsError: [...]

We already automatically combine the strides all the way to the most deeply nested parent, but we don't have the proper semantics here to access that buffer properly (which is why it's not part of the indexing pipeline yet).
The point of the "layouts" PRs has been that we need a way to strip apart the index and buffer components at each layer of a nested array and then combine them into an optimized representation.

IMO, unsafe_load(pointer(A)) == first(A), which is true for SubArrays because pointer (and memory_reference) offset the pointer when called so that this invariant holds.

Under the perspective that the base pointer of a SubArray is the pointer to its first element (rather than the pointer to the parent), then offset1 currently does return the linear offset.
And for A::OffsetArray{T,N} where N > 1, we do not have that offset1(A) == first(offsets(A)).

I'm completely fine with that perspective, but it I'm not sure when we'd ever need a unique value for the offset1 field of StrideIndex.
We'd just treat the method offset1 like Base.axes1 where it always redirects to the first dimension (except for zero dims).

If these arrays are all strided, I'd rather have a single representation capable of describing any of them.

It's been a while, but in an old PR we discussed how we needed to be able to account for non-strided indexing too. We don't need to solve every corner case, but we should probably provide standard ArrayIndex representations that can be combined in other packages.

I should probably just do a small PR that provides the few ArrayIndex subtypes I'm trying to describe right now.

I am happy with the current semantics, I just wanted to elliminate redundant data storage.
LoopVectorization tries to pass data in registers instead of on the stack, but the number of registers you can use for this is very limited.

If there's a situation where we really need a unique offset1 field then I think this PR looks good to merge, but if we don't need the field it might be simpler to just get rid of it entirely.

[chriselrod]

Neither A[StrideIndex(A)[inds...]] nor parent(A)[StrideIndex(A)[inds...] is technically the current way this functions.

Should've said unsafe_load(pointer(A), StrideIndex(A)[inds...]).

julia> using ArrayInterface

julia> A = zeros(3, 4, 5);

julia> A[:] = 1:60;

julia> Ap = @view(PermutedDimsArray(A,(3,1,2))[:,1:2,1])';

julia> ap_index = ArrayInterface.StrideIndex(Ap);

julia> ap_index[2,2] == 14 == Ap[2, 2]
true

julia> unsafe_load(pointer(Ap), ap_index[2,2])
14.0

Which is close to the behavior I actually care about.

"close", because of course I'm using memory_reference instead of pointer.

I'm completely fine with that perspective, but it I'm not sure when we'd ever need a unique value for the offset1 field of StrideIndex.
We'd just treat the method offset1 like Base.axes1 where it always redirects to the first dimension (except for zero dims).

Mostly, it makes this easier to work with:

julia> using OffsetArrays

julia> A = OffsetArray(rand(3,4), 4, 4)
3×4 OffsetArray(::Matrix{Float64}, 5:7, 5:8) with eltype Float64 with indices 5:7×5:8:
 0.695104  0.0620855  0.374289  0.971638
 0.889623  0.765056   0.812169  0.820055
 0.693763  0.876049   0.084865  0.819834

julia> Base.axes1(A)
OffsetArrays.IdOffsetRange(values=5:7, indices=5:7)

julia> ArrayInterface.offset1(A)
static(1)

julia> ArrayInterface.offsets(A)
(5, 5)

Currently, the code for index canonicalization is quite awkward, because offsets(A)[1] may or may not correspond to the first offset, and LoopVectorization cannot tell syntactically. It also wants to make use of this information outside of the @generated function (meaning within the macro) to clean things up before forwarding to the possibly not inlined @generated function.
But, I wouldn't be opposed to just dropping offset1, because that awkward code works for now so no point replacing it.

If there's a situation where we really need a unique offset1 field then I think this PR looks good to merge, but if we don't need the field it might be simpler to just get rid of it entirely.

I'm fine with either option.

[chriselrod]

It's been a while, but in an old PR we discussed how we needed to be able to account for non-strided indexing too. We don't need to solve every corner case, but we should probably provide standard ArrayIndex representations that can be combined in other packages.

I should probably just do a small PR that provides the few ArrayIndex subtypes I'm trying to describe right now.

I think using StrideIndex should be preferable whenever possible, but using these wrapper types when not possible (e.g. permuting the dims or reshaping a banded matrix), that is fine.
For the permuting dims case though, I think we should just have every type represent their dimension ranks.

Maybe stride_rank is a bad word for it, but we could view it as a permutation on the indices whenever we do cartesian indexing.

[Tokazama]

Looking over all of this I think it's clear I caused several issues with how I implemented offset1 (the method and field of StrideIndex). I think the best path forward is to have offset1(x) == offsets(x)[1]. It's nice to have for vectors, but any other interpretation seems needlessly complicated at this point.

It would be nice to have memory_reference in ManualMemory.jl so that ArrayInterface.jl can depend on it, but other issues discussed here probably don't need to be resolved in this PR.

[chriselrod]

It would be nice to have memory_reference in ManualMemory.jl so that ArrayInterface.jl can depend on it, but other issues discussed here probably don't need to be resolved in this PR.

memory_reference depends on ArrayInterface, so it isn't straightforward to move.
We could have ManualMemoryCore (which would behave like the current ManualMemory), and then another library that adds more functionality, including array handling?

I'm closing this PR in favor of #190