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.

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

[FEA] Fix improper usage of relaxed constexpr in cudf spans #17797

Open
PointKernel opened this issue Jan 23, 2025 · 0 comments
Open

[FEA] Fix improper usage of relaxed constexpr in cudf spans #17797

PointKernel opened this issue Jan 23, 2025 · 0 comments
Labels
feature request New feature or request good first issue Good for newcomers libcudf Affects libcudf (C++/CUDA) code.

Comments

@PointKernel
Copy link
Member

Is your feature request related to a problem? Please describe.
Related to #7795

While working on #17746, we noticed that some libcudf code depends on relaxed constexpr in both span and 2dspan (#17746 (comment)), e.g.

ck.fragments = &row_group_fragments.device_view()[c][f];

The above code is trying to access a device span from the host side and this should be done alternatively.

Describe the solution you'd like
For both span and 2dspan, the [] operator should be restricted to either host-only or device-only usage. However, it is valid to obtain a pointer to device storage on the host and vice versa. Therefore, instead of using the [] operator, users should access the storage pointer through data(). In the case of 2dspan, flat_view serves a similar purpose as data does in 1D spans. To align with this naming pattern, we should introduce a new API called flat_index.

CUDF_HOST_DEVICE constexpr size_type flat_index (size_type, row, size_type column) const noexcept {
  return row * this->size().second + column;
}

This approach allows us to address the aforementioned improper usage and make similar adjustments in other instances where the [] operator is misused.

Describe alternatives you've considered

Alternatively, we could follow the STL approach by introducing a mapping to our 2dspan for calculating the "flat index." However, since our 2dspan is not designed as a general-purpose solution like mdspan, it might be more appropriate to refactor the existing flat_view to align with data_handle if we choose this direction. That said, replicating the full complexity of a general-purpose implementation may not worth the effort.

@PointKernel PointKernel added feature request New feature or request good first issue Good for newcomers libcudf Affects libcudf (C++/CUDA) code. labels Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request good first issue Good for newcomers libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

No branches or pull requests

1 participant