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

Separating mdspan/mdarray infra into host_* and device_* variants #810

Merged
merged 13 commits into from
Sep 22, 2022

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Sep 7, 2022

This is a breaking change as it provides users with a more granular set of headers to import host separately from device and managed versions. It also separates the headers for mdspan and mdarray.

As an example, the following public headers can now be imported individually:

raft/core/host_mdspan.hpp
raft/core/device_mdspan.hpp

raft/core/host_mdarray.hpp
raft/core/device_mdarray.hpp

cc @rg20 @afender @akifcorduk for awareness.

Closes #806

@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 7, 2022
@cjnolet cjnolet requested a review from a team as a code owner September 7, 2022 18:48
@github-actions github-actions bot added the cpp label Sep 7, 2022
@cjnolet cjnolet added breaking Breaking change and removed non-breaking Non-breaking change labels Sep 7, 2022
Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

Just a question about .cuh vs .hpp file extensions, and whether device_ files should be .cuh?

cpp/include/raft/core/detail/mdspan_util.hpp Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member Author

cjnolet commented Sep 8, 2022

@divyegala I'm on the fence about whether the device_md{span|array} files should be in cuh files. On one hand, the default_accessor for the reference mdspan has a __device__ function but it's still in an hpp file. I think so long as the __device__ functions are kept trivial (like just for data loading), i'm inclined to keep the files as hpp for consistency. We should also be using a macro that can properly disable the __device__ in the event the compiler is not NVCC.

But, for example, if we start supplying more complex __device__ primitives that actually perform complex computations on device, or that directly use device intrinsics, I'd say we should opt to put those in files with the cuh extension (such as in mdspan_util.cuh.

cpp/include/raft/core/detail/span.hpp Outdated Show resolved Hide resolved
cpp/include/raft/core/device_mdspan.hpp Outdated Show resolved Hide resolved
cpp/include/raft/core/device_mdspan.hpp Outdated Show resolved Hide resolved
cpp/include/raft/core/device_span.hpp Show resolved Hide resolved
cpp/include/raft/core/host_mdarray.hpp Show resolved Hide resolved
cpp/include/raft/core/mdspan.hpp Show resolved Hide resolved
cpp/include/raft/core/mdspan.hpp Show resolved Hide resolved
cpp/include/raft/core/mdspan.hpp Show resolved Hide resolved
auto flatten(mdspan_type mds)
{
RAFT_EXPECTS(mds.is_exhaustive(), "Input must be contiguous.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that is_exhaustive() only means that the storage is contiguous. The range of the layout mapping need not necessarily be monotonically increasing (it could be backwards, for example).

Copy link
Member Author

@cjnolet cjnolet Sep 8, 2022

Choose a reason for hiding this comment

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

Can you provide an example of your suggested way to force this to be row or col major? (Ideally we could do this at compile time).

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a way to check if an mdspan is row or column major.

template<class ElementType, class Extents, class Layout, class Accessor>
constexpr bool is_row_or_column_major(mdspan<ElementType, Extents, Layout, Accessor> /* m */ )
{
  return false;
}

template<class ElementType, class Extents, class Accessor>
constexpr bool is_row_or_column_major(mdspan<ElementType, Extents, layout_left, Accessor> /* m */ )
{
  return true;
}

template<class ElementType, class Extents, class Accessor>
constexpr bool is_row_or_column_major(mdspan<ElementType, Extents, layout_right, Accessor> /* m */ )
{
  return true;
}

template<class ElementType, class Extents, class Accessor>
constexpr bool is_row_or_column_major(mdspan<ElementType, Extents, layout_stride, Accessor> m)
{
  return m.is_exhaustive();
}

(Relevant issue that you posted; thank you! #819 )

cpp/include/raft/core/mdspan_types.hpp Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member Author

cjnolet commented Sep 8, 2022

@mhoemmen, thank you for this thorough review! I've implemented most of your suggestions. I wasn't able to get this suggestion or this suggestion building, though most of the changes in this PR are just moving stuff around so I think we can continue to make things more clean and correct in follow-on PRs before release 22.10 is ready. Would you be interested in opening a PR w/ those changes?

@cjnolet
Copy link
Member Author

cjnolet commented Sep 9, 2022

rerun tests

Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

Love this! Had two questions on details, but this is a huge step forward for making use of mdspan in the FIL backend. I was not able to absolutely confirm that there were no gotchas with the CPU-only compilation, but everything looks good as best I can tell without finishing a version of the FIL code that makes use of this.

@cjnolet
Copy link
Member Author

cjnolet commented Sep 22, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6a1d1da into rapidsai:branch-22.10 Sep 22, 2022
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change cpp improvement Improvement / enhancement to an existing function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Separate device_mdspan and host_mdspan from mdarray.hpp into device_mdspan.hpp and host_mdspan.hpp
4 participants