Add index type template parameter to memory accessor - #2050
Open
taaae wants to merge 2 commits into
Open
Conversation
taaae
force-pushed
the
memory_accessor_add_index_type_as_template_parameter
branch
from
July 16, 2026 15:03
741c786 to
1e076e8
Compare
taaae
force-pushed
the
memory_accessor_add_index_type_as_template_parameter
branch
from
July 16, 2026 18:20
1e076e8 to
6601dc0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Things to note/discuss:
I use IndexType for the dense-matrix accessors as well:
ginkgo/common/cuda_hip/matrix/ell_kernels.cpp
Lines 245 to 246 in 741c786
ginkgo/common/cuda_hip/matrix/ell_kernels.cpp
Lines 300 to 305 in 741c786
It is useful in this line:
ginkgo/common/cuda_hip/matrix/ell_kernels.cpp
Line 108 in 741c786
But I guess it can be weird from the user's perspective. Like, they define an ELL matrix with IndexType, but suddenly dense matrices passed in the kernel also are getting accessed with this IndexType.
I also added bunch of asserts, like this one:
ginkgo/reference/matrix/ell_kernels.cpp
Lines 46 to 48 in 741c786
To prevent such overflows:
ginkgo/reference/matrix/ell_kernels.cpp
Lines 49 to 52 in 741c786
Also, the default matrix IndexType is int32:
ginkgo/include/ginkgo/core/matrix/ell.hpp
Lines 52 to 53 in 741c786
while the accessor's internal index type was hardcoded to int64 before this change:
ginkgo/accessor/reduced_row_major.hpp
Line 72 in 8e9859e
So after this change, all users of default-constructed matrices silently switch from int64 to
int32 indexing in these kernels. Do we have cases where people use matrices/operands big enough that int32 would overflow?
Small note: I use IndexType for strides and lengths as well:
ginkgo/accessor/reduced_row_major.hpp
Lines 63 to 64 in 741c786
ginkgo/accessor/reduced_row_major.hpp
Lines 151 to 154 in 741c786
It is useful because it turns the 'stored stride type' -> 'kernel index type' cast into a no-op:
ginkgo/accessor/reduced_row_major.hpp
Lines 263 to 265 in 741c786
ginkgo/accessor/reduced_row_major.hpp
Lines 169 to 173 in 741c786
ginkgo/accessor/accessor_helper.hpp
Lines 57 to 60 in 741c786
If the members stayed
size_type, thatstatic_cast<IndexType>(stride[dim_idx])would be a real64 -> 32 bit conversion on every single access.
No similar improvements for
dim_typeandlength(), but I think IndexType fits better there.