Refactor rmm::device_scalar in terms of rmm::device_uvector
#789
Conversation
This file contains 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
harrism
added
breaking
harrism
changed the title
rmm::device_scalar in terms of rmm::device_uvector
rongou
reviewed
Jun 1, 2021
| T const &initial_value, | ||
| cuda_stream_view stream = cuda_stream_view{}, | ||
| value_type const &initial_value, | ||
| cuda_stream_view stream, |
There was a problem hiding this comment.
Why remove the default? It seems that it would be a common use case to use the default stream, especially with PTDS enabled.
There was a problem hiding this comment.
Because we want to make all stream-ordered APIs explicit. See e.g. #418. Note that device_uvector and now device_buffer have no default stream parameters.
| RMM_CUDA_TRY( | ||
| cudaMemcpyAsync(element_ptr(element_index), &v, sizeof(v), cudaMemcpyDefault, s.value())); | ||
| s.synchronize_no_throw(); | ||
| if constexpr (std::is_fundamental<value_type>::value) { |
There was a problem hiding this comment.
Maybe this can be done using SFINAE?
There was a problem hiding this comment.
It certainly can, but why would I want to go back to SFINAE? I replaced SFINAE with if constexpr, which allows having a single implementation of this function rather than three!
jrhemstad
reviewed
Jun 2, 2021
include/rmm/device_scalar.hpp
Outdated
Comment on lines
40
to
46
| using value_type = T; | ||
| using reference = value_type &; | ||
| using const_reference = value_type const &; | ||
| using pointer = value_type *; | ||
| using const_pointer = value_type const *; | ||
| using iterator = pointer; | ||
| using const_iterator = const_pointer; |
There was a problem hiding this comment.
I would define these in terms of device_uvector. Also, I don't think the iterator typedefs are needed for device_scalar?
Suggested change
| using value_type = T; | |
| using reference = value_type &; | |
| using const_reference = value_type const &; | |
| using pointer = value_type *; | |
| using const_pointer = value_type const *; | |
| using iterator = pointer; | |
| using const_iterator = const_pointer; | |
| using value_type = typename device_uvector<T>::value_type; | |
| using reference = typename device_uvector<T>::reference; | |
| using const_reference = typename device_uvector<T>::const_reference; | |
| using pointer = typename device_uvector<T>::pointer; | |
| using const_pointer = typename device_uvector<T>::const_pointer; |
There was a problem hiding this comment.
I had been thinking about whether it would be useful for device_scalar to provide begin() and end()...
jrhemstad
reviewed
Jun 2, 2021
jrhemstad
reviewed
Jun 2, 2021
rongou
approved these changes
Jun 3, 2021
jrhemstad
approved these changes
Jun 3, 2021
|
@gpucibot merge |
rapids-bot bot
pushed a commit
to rapidsai/cudf
that referenced
this pull request
Jun 8, 2021
rapidsai/rmm#789 refactors `rmm::device_scalar`, which all of `cudf::scalar` depends on. Notably, it renames some methods, makes stream parameters explicit, and deletes streamless constructors. As a result, the present PR deletes the default and non-stream copy constructors of all the `cudf::*_scalar` classes. This should be merged immediately after rapidsai/rmm#789 because that PR will break the build. Authors: - Mark Harris (https://github.com/harrism) Approvers: - https://github.com/brandon-b-miller - Vukasin Milovanovic (https://github.com/vuule) - Robert (Bobby) Evans (https://github.com/revans2) - Robert Maynard (https://github.com/robertmaynard) URL: #8411
rapids-bot bot
pushed a commit
to rapidsai/raft
that referenced
this pull request
Jun 8, 2021
In comms/test.hpp `device_scalar::value` was not being passed an explicit stream, which means that the default stream was being synced. rapidsai/rmm#789 will remove the default from this parameter, and would have therefore broken the RAFT build. So this PR fixes the oversynchronization and ensures RAFT will build after the RMM PR is merged. Note this PR includes the cmake changes from #258 (just so I could build locally). Once #258 is merged this PR's changes will be simplified. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) - Chuck Hastings (https://github.com/ChuckHastings) URL: #259
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.
This PR refactors
device_scalarto use a single-elementdevice_uvectorfor its storage. This simplifies the implementation of device_scalar. Also changes the API ofdevice_scalarso that its asynchronous / stream-ordered methods use the same API style (with explicit stream parameter) asdevice_uvectoranddevice_buffer.Closes #570
This is a breaking change. When it is merged, PRs are likely to need to be merged immediately in other libraries to account for the API changes.
cudf::scalarclasses to changes inrmm::device_scalarcudf#8411rmm::device_scalarAPI cugraph#1637cuML(unused)cuSpatial(unused)