Skip to content
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.

Sign up for GitHub

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

Use offsetalator in cudf::strings::reverse #15001

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Use offsetalator in cudf::strings::reverse #15001

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions cpp/src/strings/reverse.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <cudf/column/column_device_view.cuh>
#include <cudf/column/column_factories.hpp>
#include <cudf/detail/nvtx/ranges.hpp>
#include <cudf/detail/offsets_iterator_factory.cuh>
#include <cudf/strings/detail/utf8.hpp>
#include <cudf/strings/reverse.hpp>
#include <cudf/strings/string_view.cuh>
Expand All @@ -37,7 +38,7 @@ namespace {
*/
struct reverse_characters_fn {
column_device_view const d_strings;
size_type const* d_offsets;
cudf::detail::input_offsetalator d_offsets;
char* d_chars;

__device__ void operator()(size_type idx)
Expand All @@ -62,10 +63,10 @@ std::unique_ptr<column> reverse(strings_column_view const& input,
if (input.is_empty()) { return make_empty_column(type_id::STRING); }

// copy the column; replace data in the chars column
auto result = std::make_unique<column>(input.parent(), stream, mr);
auto const d_offsets =
result->view().child(strings_column_view::offsets_column_index).data<size_type>();
auto d_chars = result->mutable_view().head<char>();
auto result = std::make_unique<column>(input.parent(), stream, mr);
auto sv = strings_column_view(result->view());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could result be const here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do modify the column, const would be a bit misleading IMO 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result should be elided on return which is kind of like a move which changes the object which would be prevented on a const object. I'd rather communicate to the compiler (and the reviewers) that this object is moved and not copied on return. I fear the const declaration would send the wrong hint to the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, constness does not interfere with the copy elision, e.g. https://godbolt.org/z/TzPjshqrd
We'd need a std::move in the return if the copy elision didn't kick in, since result is not copyable.

I agree that const sends the wrong message regardless.

auto const d_offsets = cudf::detail::offsetalator_factory::make_input_iterator(sv.offsets());
auto d_chars = result->mutable_view().head<char>();

auto const d_column = column_device_view::create(input.parent(), stream);
thrust::for_each_n(rmm::exec_policy(stream),
Expand Down
Loading