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
Add a host-pinned memory resource that can be used as upstream for
pool_memory_resource
. #1392Add a host-pinned memory resource that can be used as upstream for
pool_memory_resource
. #1392Changes from 48 commits
fae33fa
15be572
e8c227b
2b37372
c43a8c1
d238daa
3d65d4c
66d85b4
265de9b
0be364b
266afa9
5d66f40
fae5b73
92c0653
2acf759
a70b24e
b6edcd1
782ff55
4ef844a
ce58ff5
0b4c968
2f827a5
4f91478
f581809
bafd70a
a77d215
07dffa3
8afff2d
0140bd4
91752c8
baf429c
c90e81c
c2843be
6e0aeaa
c3c61e1
014ac5b
909b733
0fc3fba
9a876b5
b819738
27fe52c
da934ba
7d51fea
85286b0
f7b0ca5
52fc2f1
6162699
fa140ae
aafa18a
92c8e23
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.
This docstring implies it's possible to compare with another type of resource and get
false
, but the implementation doesn't allow that. Do we need to update the implementation or the docstrings?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.
Oh yeah, I had that thought. Is there a blanket "false" implementation in the base class somehow?
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 think this is how comparison works with
cuda::mr
. Basically if you try to compare with another type of resource, compilation will fail. Note that refactoring tocuda::mr
will necessitate changing the semantics RMM currently (mostly) has for MR equality comparison. #1402There 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.
Note also that
pinned_host_memory_resource
is NOT adevice_memory_resource
. It simply implements thecuda::mr::memory_resource
andcuda::mr::async_memory_resource
concepts.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.
(also note there is no base class)
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 changed the docstring so it doesn't say that false can be returned. Note that we should probably followup with more explicit tests of this MR and future MRs like it. Right now, though, our test machinery for MRs assumes they are all
device_memory_resource
, so while I can pass apool_memory_resource<pinned_host_memory_resource>
to all the MR tests, I can't pass justpinned_host_memory_resource
currently. (It does get tested as the upstream in the former case though, including itsoperator==
).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.
Okay. If there's no base class, I've just lost track of how the class hierarchy works. I don't have any further comments here but I'll need to refresh myself on how things are supposed to work someday.