Fix cudf::test::to_host to handle both offset types for strings columns #15073
Conversation
davidwendt
added
2 - In Progress
davidwendt
changed the title
davidwendt
added
3 - Ready for Review
| CUDF_CUDA_TRY( | ||
| cudaMemcpy(host_rep_types.data(), c.begin<Rep>(), c.size() * sizeof(Rep), cudaMemcpyDefault)); |
There was a problem hiding this comment.
Can we use cudf::detail::make_std_vector_sync or some vector factory like that, rather than using raw cudaMemcpy calls? We're using that function below, so it seems reasonable to do the same here.
There was a problem hiding this comment.
This cudaMemcpy was not introduced here but merely moved from .hpp file.
All of the to_host overloads return thrust::host_vector objects and the vector factories return std::vector.
I think the right thing would be change all of these since there are more than this one cudMemcpy due to the to_host signature. I believe this would be out of scope for this PR and I can create follow on one to correct all the to_host functions and callers.
There was a problem hiding this comment.
Great. Let's do a follow-up PR.
| auto const host_fixed_points = thrust::host_vector<T>(begin, begin + c.size()); | ||
|
|
||
| return {host_fixed_points, bitmask_to_host(c)}; |
There was a problem hiding this comment.
nit: copy elision does not happen here. (for both const or non-const)
Tried a few codes in compiler explorer; https://godbolt.org/z/jMGn8qK31
Found that copy elision (NRVO) doesn't work while calling constructor of std::pair.
It's probably better to usestd::move() here.
It could be a follow up PR to optimize all specializations in this file.
|
/merge |
Cleanup per comments in #15073: - Fix return to move instead of copy https://github.com/rapidsai/cudf/pull/15073/files#r1507913472 - Use vector factories instead of cudaMemcpy: https://github.com/rapidsai/cudf/pull/15073/files#r1500136815 Also removed some unneeded headers found in `gather.cuh` while working on this. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #15263
Description
The
cudf::test::to_hostfunction is updated to handle int32 and int64 offset types for strings columns when copying data to host memory. This function is used withcudf::test::print()as well.Also moved the function from the header
column_utilities.hppto thecolumn_utilities.cufile.And moved the specialization for of
to_hostfor fixed-point types from the header to.cuas well.Checklist