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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating
raft::linalg
APIs to usemdspan
#809Updating
raft::linalg
APIs to usemdspan
#809Changes from 18 commits
139eaf4
ddfffc3
362beef
5ef4391
be85cac
c5abca1
42506bf
4ea680d
bbd5cbe
a5fd419
622ed75
8ccf266
2f1df6c
536bea4
9b032ce
2791324
47b3941
ca3b601
fb4e6eb
d737ffd
c083b3b
ec57e0d
e6f6373
a3d4993
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the goal here is to use 32-bit indices if possible, when inverting the layout mapping to use a 1-D loop index. This can be done, but there are two correctness issues with your approach.
The right quantity to test here is
out.required_span_size()
, notout.size()
. The layout mapping maps the input multidimensional index to the half-open interval of offsets [0,out.required_span_size()
).The
layout_{left, right, stride}::mapping
constructors generally have as a precondition that the required span size of the input extents (and strides, if applicable) be representable as a value of typeindex_type
.Here is an approach that would address these issues.
You'll also want to check the
index_type
andrequired_span_size()
of the other mdspan. The above approach has the advantage that it only compiles an inner kernel for index types that you actually use.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In point 2, what happens in extreme cases? Consider
index_type=uint32_t
with extents{2^32, 2}
. In this case, willrequired_span_size()
by representable byindex_type
or will it cause an overflow?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@divyegala
required_span_size()
is not representable byindex_type
in this case. Forlayout_left
andlayout_right
,required_span_size()
andsize()
are the same mathematically. The only difference is the return type (index_type
resp.size_t
). Forlayout_stride
, though,required_span_size()
can be greater than thesize()
. For other layouts (e.g., the "matrix of a single value" layout that maps all multidimensional indices to the offset zero),required_span_size()
can be less thansize()
.Note that while it's UB for users to violate preconditions, implementations aren't required to check preconditions. The reference implementation of
layout_left
does not currently check preconditions, as you can see here, for instance. This means two things.If someone gives you a
layout_{left,right,stride}::mapping
instance (e.g., in anmdspan
), then you can assume that the precondition is satisfied.If you are constructing a
layout_{left,right,stride}::mapping
instance (e.g., by constructing anmdspan
with a pointer and extents), then you are responsible for ensuring that the precondition is satisfied.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@divyegala wrote:
Those are two separate questions, actually! : - )
required_span_size()
is not representable byindex_type
in this case.extents
object tolayout_{left,right,stride}::mapping
's constructor violates the constructor's precondition. It could overflow, or it could open a portal to the Awesome Dimension and let loose a swarm of nasal demons who search out precondition violators and boop them gently on the nose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhoemmen thanks for the explanations! How do we really represent such edge cases and safely obtain the product of the extents? Sounds like
size()
is the safe way to obtain the product without violating any pre-conditions since it's representable bysize_t
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@divyegala Gladly! : - )
By the time the user has created a layout mapping, it's already too late. What I mean by that is that if
required_span_size()
doesn't fitindex_type
, then the user will likely get the wrong answer when they try to index into the mdspan.In what follows in my comment, I'll distinguish between "the Preconditions in the spec" and "what the reference implementation does." The reference implementation currently does not check this precondition in the layout mapping. This means that it's possible for users to construct extents for which the mapping's
required_span_size()
can overflow.We can prevent this by wrapping mdspan creation to check the extents object for potential overflow, before it goes into a layout mapping's constructor. It's not UB to construct, e.g.,
dextents<uint16_t, 2>(
2^{15},
2^{15})
. We just need to intercept that naughtyextents
value before it goes into a layout mapping's constructor. Otherwise, the layout mapping has the freedom to do whatever it likes, including callingabort()
.Our mdarray implementation's conversion to mdspan can also check, but again, we're probably better off making the wrapper explicit and not part of the mdarray proposal. WG21 likes Preconditions and wants violating them to be UB. If we want some specified behavior (e.g., throwing a particular exception, or calling
terminate()
after printing a helpful error message), then we'll have to implement that ourselves.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like
out.required_span_size()
does not work. How do I access this from the layout?