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
Host/Device accessors for
mdspan
#3686base: main
Are you sure you want to change the base?
Host/Device accessors for
mdspan
#3686Changes from 23 commits
b115039
0ea452d
9ae4953
96f8419
52b12a6
cab748e
31b84c7
430e46d
3c76d32
d63980d
e9ce8a4
de4264c
c0d12da
572447d
0eb90ef
71a9161
921273a
aef9602
0647703
c30482e
fc82009
21e2feb
22fecf3
ca47b44
9a7e393
a0af8f4
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.
In a previous review, I mentioned the issue that this omits all the conversions to and from
_Accessor
that the_Accessor
class defines. I just want to mention it again so I don't forget : - ) . Conversions are an important part of an accessor's interface, because they help define conversions for mdspan itself. For example, withoutdefault_accessor
's converting constructor, you can't assign mdspan-of-nonconst to mdspan-of-const. Section 5.8 of P2897 elaborates this explanation.Should we consider instead a CRTP base class approach to
__*_accessor
? That would let developers of custom accessors opt into the run-time pointer checking functionality.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.
could be that you reviewed an older version? I previously added conversions from/to
default_accessor
forhost_accessor
anddevice_accessor
.Honestly, I'm was sure about
managed_accessor
because it looks unsafe...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.
the idea is nice. I'm a bit concerned about the implications for users. Could
host_accessor
be roughly defined in the following way?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.
@fbusato wrote:
The issue is with transitive conversions for a possibly generic
_Accessor
.For example, suppose that I define two accessors
A
andB
.A
is weaker (more type-erased) thanB
. A defines an implicit conversion fromB
, andB
defines an explicit conversion fromA
. (This follows the idiom that explicit conversions assert preconditions, while implicit conversions have no preconditions.) One example of this case isA = default_accessor<float>
, andB = aligned_accessor<float, 16>
.The problem is, these conversions don't carry over to
{host,device,managed}_accessor
. For example,host_accessor<A>
does not have an implicit conversion fromhost_accessor<B>
, andhost_accessor<B>
does not have an explicit conversion fromhost_accessor<A>
.Here's a draft for how to fix this. I would need to prototype this a bit to make sure it works.
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.
@fbusato I just sent you an offline message with a link to an example of how to do this correctly.
wrapper
(see below) is likehost_accessor
ordevice_accessor
-- an accessor that wraps another accessor. The link explains the design and includes tests and use examples.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.
A key feature of any accessor is the set of permitted conversions, both implicit and explicit. mdspan depends on accessors' conversions in order for its own conversions to work. Implicit conversions make mdspan assignment work; explicit conversions make explicit construction of an
mdspan
of one type from an mdspan of another type work. Evendefault_accessor
has a conversion to permit assigning an mdspan of nonconst to an mdspan of const.An accessor's conversions are defined by its constructors and/or conversion operators. Not exposing those from the parent class means that mdspan of
host_accessor<Accessor>
won't be able to perform any of the conversions that mdspan ofAccessor
can perform.A public
using _Accessor::_Accessor;
declaration would pull in all of the base class' constructors, but it wouldn't pull in any conversion operators.Another issue is that
_Accessor
might have a virtual destructor. That would be weird, but it's legal in the Standard and nothing in this class prevents it.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.
Section 5.8 of P2897R7 elaborates the design intent behind accessor conversions.
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.
For example, if Accessor A provides a conversion operator to B and a conversion operator to C, the only way to provide those in
device_accessor<A>
would be to use reflection to enumerate all conversion operators of A, and paste new ones intodevice_accessor<A>
fordevice_accessor<B>
anddevice_accessor<C>
. While I'm not a C++ reflection expert, I don't think even P2996 reflection can do that, as it would require injecting member function definitions.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.
I entirely missed that part. I added the constructors following the
aligned_accessor
model.