Skip to content

Commit

Permalink
Fix cudf::test::to_host return of host_vector (#15263)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
davidwendt authored Mar 12, 2024
1 parent 155405b commit a4e73a5
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 23 deletions.
2 changes: 0 additions & 2 deletions cpp/include/cudf/detail/gather.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@

#include <thrust/functional.h>
#include <thrust/gather.h>
#include <thrust/host_vector.h>
#include <thrust/iterator/counting_iterator.h>
#include <thrust/iterator/transform_iterator.h>
#include <thrust/logical.h>

#include <algorithm>
Expand Down
6 changes: 3 additions & 3 deletions cpp/include/cudf_test/column_utilities.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,9 @@ bool validate_host_masks(std::vector<bitmask_type> const& expected_mask,
template <typename T, std::enable_if_t<not cudf::is_fixed_point<T>()>* = nullptr>
std::pair<thrust::host_vector<T>, std::vector<bitmask_type>> to_host(column_view c)
{
thrust::host_vector<T> host_data(c.size());
CUDF_CUDA_TRY(cudaMemcpy(host_data.data(), c.data<T>(), c.size() * sizeof(T), cudaMemcpyDefault));
return {host_data, bitmask_to_host(c)};
auto col_span = cudf::device_span<T const>(c.data<T>(), c.size());
auto host_data = cudf::detail::make_host_vector_sync(col_span, cudf::get_default_stream());
return {std::move(host_data), bitmask_to_host(c)};
}

// This signature is identical to the above overload apart from SFINAE so
Expand Down
32 changes: 14 additions & 18 deletions cpp/tests/utilities/column_utilities.cu
Original file line number Diff line number Diff line change
Expand Up @@ -906,20 +906,18 @@ void expect_column_empty(cudf::column_view const& col)
std::vector<bitmask_type> bitmask_to_host(cudf::column_view const& c)
{
if (c.nullable()) {
auto num_bitmasks = num_bitmask_words(c.size());
std::vector<bitmask_type> host_bitmask(num_bitmasks);
if (c.offset() == 0) {
CUDF_CUDA_TRY(cudaMemcpy(host_bitmask.data(),
c.null_mask(),
num_bitmasks * sizeof(bitmask_type),
cudaMemcpyDefault));
} else {
auto num_bitmasks = num_bitmask_words(c.size());
auto [bitmask_span, _] = [&] {
if (c.offset() == 0) {
return std::pair{cudf::device_span<bitmask_type const>(c.null_mask(), num_bitmasks),
rmm::device_buffer{}};
}
auto mask = copy_bitmask(c.null_mask(), c.offset(), c.offset() + c.size());
CUDF_CUDA_TRY(cudaMemcpy(
host_bitmask.data(), mask.data(), num_bitmasks * sizeof(bitmask_type), cudaMemcpyDefault));
}

return host_bitmask;
return std::pair{cudf::device_span<bitmask_type const>(
static_cast<bitmask_type*>(mask.data()), num_bitmasks),
std::move(mask)};
}();
return cudf::detail::make_std_vector_sync(bitmask_span, cudf::get_default_stream());
} else {
return std::vector<bitmask_type>{};
}
Expand All @@ -946,16 +944,14 @@ std::pair<thrust::host_vector<T>, std::vector<bitmask_type>> to_host(column_view
using namespace numeric;
using Rep = typename T::rep;

auto host_rep_types = thrust::host_vector<Rep>(c.size());

CUDF_CUDA_TRY(
cudaMemcpy(host_rep_types.data(), c.begin<Rep>(), c.size() * sizeof(Rep), cudaMemcpyDefault));
auto col_span = cudf::device_span<Rep const>(c.begin<Rep>(), c.size());
auto host_rep_types = cudf::detail::make_host_vector_sync(col_span, cudf::get_default_stream());

auto to_fp = [&](Rep val) { return T{scaled_integer<Rep>{val, scale_type{c.type().scale()}}}; };
auto begin = thrust::make_transform_iterator(std::cbegin(host_rep_types), to_fp);
auto const host_fixed_points = thrust::host_vector<T>(begin, begin + c.size());

return {host_fixed_points, bitmask_to_host(c)};
return {std::move(host_fixed_points), bitmask_to_host(c)};
}

template std::pair<thrust::host_vector<numeric::decimal32>, std::vector<bitmask_type>> to_host(
Expand Down

0 comments on commit a4e73a5

Please sign in to comment.